Support flush reasons above 12 in Java integration

[rpuch]

Summary:

FlushReason enum in C++ has members up to 15, but in Java, the mirroring FlushReason only supports reason codes up to 12. This causes exceptions when adding a flush listener.

[rpuch]

Here is what can be seen on the console (probably, it's stderr) during flushes:

java.lang.IllegalArgumentException: Illegal value provided for FlushReason: 13
        at org.rocksdb.FlushReason.fromValue(FlushReason.java:51)
        at org.rocksdb.FlushJobInfo.<init>(FlushJobInfo.java:41)

[alanpaxton]

Hi @rpuch - that looks good. How hard would it be to add a unit test which will fail again if another value is added to the enum in future ?

I'm sure you are aware that you will need to rebase before the RocksDB Core Team will look at merging your change.

[rpuch]

Hi @rpuch - that looks good. How hard would it be to add a unit test which will fail again if another value is added to the enum in future ?

I'm sure you are aware that you will need to rebase before the RocksDB Core Team will look at merging your change.

Hi Alan, thanks for your review. I will try to add such a test, but right now I'm not even sure how to start. Should the test be in Java? If yes, what is the recommended way to obtain max FlushReason value defined in C++? If not in Java, do you have other suggestions?

As for rebasing: sure, I'll do it. But does this rule mean that after any commit gets merged to main, my PR will immediately become stale and will need to be rebased again? Is there a procedure to avoid a potentially long chain of rebases while chasing the fresh main?

[alanpaxton]

Hi @rpuch - I'm not sure if a unit test needs to know the maximum. Just something that calls fromValue() in the way your code does, will fail when it finally sees a new, larger value ?

Let's leave the rebase until we are happy with your PR, then do it once, and you will probably be asked to do it once more by the RocksDB Core Team when they are ready to pull it into their system.

[rpuch]

Hi @rpuch - I'm not sure if a unit test needs to know the maximum. Just something that calls fromValue() in the way your code does, will fail when it finally sees a new, larger value ?

Let's leave the rebase until we are happy with your PR, then do it once, and you will probably be asked to do it once more by the RocksDB Core Team when they are ready to pull it into their system.

The thing is that it's not my code that calls fromValue(). RocksDB Java integration does this itself, see https://github.com/facebook/rocksdb/blob/main/java/src/main/java/org/rocksdb/FlushJobInfo.java#L41

This is the problem I'm trying to fix. I could write a unit test making sure that FlushJobInfo could be instantiated with reason codes up to 15, but this will not help if a new code is added in the C++ part, but gets forgotten in the Java part.

[alanpaxton]

Hi @rpuch - I'm not sure if a unit test needs to know the maximum. Just something that calls fromValue() in the way your code does, will fail when it finally sees a new, larger value ?
Let's leave the rebase until we are happy with your PR, then do it once, and you will probably be asked to do it once more by the RocksDB Core Team when they are ready to pull it into their system.

The thing is that it's not my code that calls fromValue(). RocksDB Java integration does this itself, see https://github.com/facebook/rocksdb/blob/main/java/src/main/java/org/rocksdb/FlushJobInfo.java#L41

This is the problem I'm trying to fix. I could write a unit test making sure that FlushJobInfo could be instantiated with reason codes up to 15, but this will not help if a new code is added in the C++ part, but gets forgotten in the Java part.

Ah sorry, I misunderstood. Then perhaps the best thing is simply to add a comment to the C++ enum, indicating that any new values should be added to Java also.

[alanpaxton]

Thanks @rpuch that all looks good to me. @jaykorean could you or someone else on the team import this one ?

[facebook-github-bot]

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.