Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add jni Support for API CreateColumnFamilyWithImport #11646
Add jni Support for API CreateColumnFamilyWithImport #11646
Changes from 5 commits
8dcc9a5
ee327e0
cf810a4
57e2b16
710fd7c
bf29361
db02ebe
b8b6783
4520b64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused between the relationship you have chosen between the Java class
org.rocksdb.ExportImportFilesMetaData
and the C++ classROCKSDB_NAMESPACE::ExportImportFilesMetaData
.Normally where there is a Java class that may need to have ownership of a C++ object, we typically follow the pattern of inheriting from RocksObject where possible, or if you need references to C++ objects which should not be owned by the Java Object then you may extend AbstractImmutableNativeReference.
I am wondering if there is a reason why you have chosen not to follow that pattern? It is important in RocksJava that we understand the ownership model so that we don't leak memory by not deleting C++ objects that we own, and/or causing crashes by deleting C++ objects that we don't own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamretter
Thank you very much for Review code, it is my honor to contribute code.
I‘v read your comment. Before I modify the code I would like to ask a question, PTAL
ExportImportFilesMetaData may be used in two places. One situation is that we can create ExportImportFilesMetaData through Checkpoint.exportColumnFamily, from here it seems that org.rocksdb.ExportImportFilesMetaData and org.rocksdb.LiveFileMetaData are similar, Called from JNI C++, so i did not to follow that pattern.
But in another case, ExportImportFilesMetaData needs to be passed in when calling createColumnFamilyWithImport. In this case we need to follow the pattern of inheriting from RocksObject.
So if this is the case, do LiveFileMetaData and SstFileMetaData also need to follow this pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in C++ when you call
Checkpoint::ExportColumnFamily
the caller will be the owner of the newExportImportFilesMetaData
object that is created and returned. So if you are calling this from Java, then the Java caller is now the owner of this, and is responsible for freeing it - for this something likeRocksObject
in Java is perfect.In C++ when you call
DB::CreateColumnFamilyWithImport
as the caller you have to provide anExportImportFilesMetaData
object. As you are in C++ you may choose to create this object either on the stack or on the heap. From Java though if you want to have a Java object which represents this C++ object, then you will need to call JNI and create the object on the heap, at which point you are the owner of that object, and so therefore againRocksObject
in Java again becomes the correct choice.@mayuehappy Is that helpful?