Fix exception on RocksDB.getColumnFamilyMetaData() #12474
Conversation
| } | ||
|
|
||
| try (final RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { | ||
| ColumnFamilyMetaData metadata = db.getColumnFamilyMetaData(); // Exception here |
There was a problem hiding this comment.
In the reproduction that I had, it was only the second call to getColumnFamilyMetaData that would throw an exception. We might consider testing that case as well. But does this code also fail on 8.11.fb and 9.0.fb?
There was a problem hiding this comment.
But does this code also fail on 8.11.fb and 9.0.fb
You would need to test it. We are unlikely to backport this directly
There was a problem hiding this comment.
Hello @neilramaswamy,
I was able to get the exception on first call. Didn't need the second one. As I mentioned, what is importand is the SST file on disk. This was the trigger for exception.
Radek
There was a problem hiding this comment.
You would need to test it. We are unlikely to backport this directly
Yeah, I was more curious, wasn't expecting a backport. But I did verify that on 8.11.3 and 9.0.0 this test indeed fails. Thanks Radek!
I'm curious though, @adamretter, is this fix something that you would backport? I imagine that breaking getColumnFamilyMetaData calls for an SST file on disk could impact a lot of users.
|
@rhubner, is this ready for review? |
|
Hello @neilramaswamy,
Yes it is. Radek |
rhubner
force-pushed
the
cf-medatada-fix
branch
from
aaa12ef
to
297da9d
Compare
|
Perhaps @adamretter or @ajkr could take a look? I also manually verified that these changes fix the original repro as I reported in #12466. |
|
@ajkr @pdillinger Please could you merge this to:
|
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: #12466 reported a bug when `RocksDB.getColumnFamilyMetaData()` is called on an existing database(With files stored on disk). As neilramaswamy mentioned, this was caused by #11770 where the signature of `SstFileMetaData` constructor was changed, but JNI code wasn't updated. This PR fix JNI code, and also properly populate `fileChecksum` on `SstFileMetaData`. Pull Request resolved: #12474 Reviewed By: jowlyzhang Differential Revision: D55811808 Pulled By: ajkr fbshipit-source-id: 2ab156f41eaf4a4f30c49e6df421b61e8451230e
Summary: #12466 reported a bug when `RocksDB.getColumnFamilyMetaData()` is called on an existing database(With files stored on disk). As neilramaswamy mentioned, this was caused by #11770 where the signature of `SstFileMetaData` constructor was changed, but JNI code wasn't updated. This PR fix JNI code, and also properly populate `fileChecksum` on `SstFileMetaData`. Pull Request resolved: #12474 Reviewed By: jowlyzhang Differential Revision: D55811808 Pulled By: ajkr fbshipit-source-id: 2ab156f41eaf4a4f30c49e6df421b61e8451230e
|
Thanks for the backport, Andrew! @adamretter are you the one who creates the artifact? |
|
@neilramaswamy it depends which artifact you mean? I built and published the binaries for RocksJava 8.11.4 to Maven Central yesterday. |
|
@adamretter, Ah, when I posted that it hadn't shown up on Maven Central yet. Thanks! |
|
@ajkr, FYI that I think that this might have been forgotten to be cherry-picked into 9.1.0. I see it in 8.11.fb and 9.0.fb, though. |
Summary: #12466 reported a bug when `RocksDB.getColumnFamilyMetaData()` is called on an existing database(With files stored on disk). As neilramaswamy mentioned, this was caused by #11770 where the signature of `SstFileMetaData` constructor was changed, but JNI code wasn't updated. This PR fix JNI code, and also properly populate `fileChecksum` on `SstFileMetaData`. Pull Request resolved: #12474 Reviewed By: jowlyzhang Differential Revision: D55811808 Pulled By: ajkr fbshipit-source-id: 2ab156f41eaf4a4f30c49e6df421b61e8451230e
|
@neilramaswamy Thanks for pointing that out. I will fix that now. |
#12466 reported a bug when
RocksDB.getColumnFamilyMetaData()is called on an existing database(With files stored on disk). As @neilramaswamy mentioned, this was caused by #11770 where the signature ofSstFileMetaDataconstructor was changed, but JNI code wasn't updated.This PR fix JNI code, and also properly populate
fileChecksumonSstFileMetaData.