Add jni Support for API CreateColumnFamilyWithImport #11646

mayuehappy · 2023-07-27T12:21:14Z

Add the following missing options to src/main/java/org/rocksdb/ImportColumnFamilyOptions.java and in java/rocksjni/import_column_family_options.cc in RocksJava.
Add the struct to src/main/java/org/rocksdb/ExportImportFilesMetaData.java and in java/rocksjni/export_import_files_metadatajni.cc in RocksJava.
Add New Java API createColumnFamilyWithImport to src/main/java/org/rocksdb/RocksDB.java
Add New Java API exportColumnFamily to src/main/java/org/rocksdb/Checkpoint.java

Test plan:

added unit tests for exportColumnFamily in org.rocksdb.CheckpointTest
added unit tests for createColumnFamilyWithImport to org.rocksdb.ImportColumnFamilyTest

adamretter

This looks like a great start, thanks for your contribution :-)
I just have a few questions around object ownership please

adamretter · 2023-07-28T14:41:56Z

java/src/main/java/org/rocksdb/ExportImportFilesMetaData.java

+ /**
+ * Called from JNI C++
+ */
+ public ExportImportFilesMetaData(final byte[] dbComparatorName, final LiveFileMetaData[] files) {


If this is only called from JNI, it can be made private please

adamretter · 2023-07-28T14:45:21Z

java/src/main/java/org/rocksdb/ExportImportFilesMetaData.java

+ return Arrays.asList(files);
+ }
+
+ public long newExportImportFilesMetaDataHandle() {


I don't think this should be in this class. I see it is called from RocksDB. I think it would be better if the processing was done in RocksDB as the native handles are not used here.

adamretter · 2023-07-28T14:58:47Z

java/src/main/java/org/rocksdb/RocksDB.java

+ final List<ExportImportFilesMetaData> metadatas) throws RocksDBException {
+ final int metadataNum = metadatas.size();
+ final long[] metadataHandeList = new long[metadataNum];
+ for (int i = 0; i < metadataNum; i++) {


I am a bit confused between the relationship you have chosen between the Java class org.rocksdb.ExportImportFilesMetaData and the C++ class ROCKSDB_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.

@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?

So in C++ when you call Checkpoint::ExportColumnFamily the caller will be the owner of the new ExportImportFilesMetaData 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 like RocksObject in Java is perfect.

In C++ when you call DB::CreateColumnFamilyWithImport as the caller you have to provide an ExportImportFilesMetaData 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 again RocksObject in Java again becomes the correct choice.

@mayuehappy Is that helpful?

mayuehappy · 2023-09-08T02:54:05Z

@adamretter @cbi42 Ping~ , Can anyone help me look at this PR?

adamretter · 2023-09-08T20:19:59Z

@mayuehappy I just replied to your comment above - let me know if you need any more guidance...

mayuehappy · 2023-09-27T10:00:27Z

@adamretter
Thanks for the guidance, I've updated the PR. Change ExportImportFilesMetaData to extends RocksObject, please help and take a look

adamretter · 2023-10-06T12:19:05Z

java/src/main/java/org/rocksdb/ExportImportFilesMetaData.java

+ private ExportImportFilesMetaData(final long nativeHandle) {
+ super(nativeHandle);
+ // We do not own the native object!
+ disOwnNativeHandle();


@mayuehappy can you explain why you are callng disOwnNativeHandle here please?

@adamretter Thanks for the reviewing, I thought about this place again. org.rocksdb.ExportImportFilesMetaData should indeed own ROCKSDB_NAMESPACE::ExportImportFilesMetaData's nativeHandle. My initial idea was a little wrong, so I deleted these two lines of code.

pnowojski · 2023-10-20T10:50:48Z

Hi @mayuehappy and @adamretter. Thank you both for your efforts so far on this and other related PRs! I'm eagerly waiting for this feature to be merged. Is there something that I could do to help pushing this PR over the line? If there are still some issues to be resolved, me or my colleagues would be happy to help.

mayuehappy · 2023-10-23T02:56:41Z

Hi @mayuehappy and @adamretter. Thank you both for your efforts so far on this and other related PRs! I'm eagerly waiting for this feature to be merged. Is there something that I could do to help pushing this PR over the line? If there are still some issues to be resolved, me or my colleagues would be happy to help.

@pnowojski Hi ~ Thank you for your attention. I alse hope this PR can be merged soon so that Flink can use this new feature to speed up state recovery!

@adamretter .If there is anything that needs to be modified in this PR, please comment and I will fix it as soon as possible.
cc @cbi42

adamretter · 2023-11-01T10:55:23Z

@mayuehappy I think this looks good. I would like my colleague @alanpaxton to also give it a quick once over too. @alanpaxton could you just check the "ownership" semantics as discussed above, I am pretty certain we have them correct now - but a second pair of eyes is always valuable.

alanpaxton

@adamretter @mayuehappy I have looked at this w.r.t. "ownership" semantics, and it appears correct to me for that. I noticed a couple of small issues in passing that I commented..

alanpaxton · 2023-11-01T11:34:49Z

java/src/main/java/org/rocksdb/RocksDB.java

+ final ImportColumnFamilyOptions importColumnFamilyOptions,
+ final List<ExportImportFilesMetaData> metadatas) throws RocksDBException {
+ final int metadataNum = metadatas.size();
+ final long[] metadataHandeList = new long[metadataNum];


Typo - metadataHandleList

alanpaxton · 2023-11-01T11:56:22Z

java/src/main/java/org/rocksdb/ImportColumnFamilyOptions.java

+/**
+ * ImportColumnFamilyOptions is used by ImportColumnFamily()
+ * <p>
+ * Note that dispose() must be called before this instance become out-of-scope


This behaves the same as any other RocksObject. If you need a specific comment, it should note that the should be close()d (since close is the public method which ultimately calls disposeInternal()), and ideally that should use a try-with-resources construct.

alanpaxton · 2023-11-01T12:05:39Z

java/rocksjni/checkpoint.cc

+ if (metadata != nullptr) {
+ jexport_import_files_meta_data =
+ ROCKSDB_NAMESPACE::ExportImportFilesMetaDataJni::
+ fromCppExportImportFilesMetaData(env, metadata);


Would it not be easier to return metadata as a jlong (GET_CPLUS_PLUS_PTR) and then call new ExportImportFilesMetaData(metadata_native_) in Java ? Then you would not need to add the extra class to portal.h, I believe ?

facebook-github-bot · 2023-11-01T17:08:52Z

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

cbi42 · 2023-11-01T17:29:37Z

LGTM, I'll merge it once the other comments are addressed.

facebook-github-bot · 2023-11-02T06:06:37Z

@mayuehappy has updated the pull request. You must reimport the pull request before landing.

mayuehappy · 2023-11-02T06:21:57Z

@cbi42 @adamretter @alanpaxton
Thanks a lot for reviewing the code, I've updated the PR as suggested by @alanpaxton . Mainly include the following changes：

fix metadataHandeList typo issues
Updated ImportColumnFamilyOptions class comment. And use try-with-resources in unit tests to create ImportColumnFamilyOptions
change Java_org_rocksdb_Checkpoint_exportColumnFamily returns types to jlong, then call new ExportImportFilesMetaData(metadata_native_) to create ExportImportFilesMetaData and delete portal.h related changes

java/src/main/java/org/rocksdb/ExportImportFilesMetaData.java

facebook-github-bot · 2023-11-03T02:43:46Z

@mayuehappy has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-11-03T16:41:31Z

@mayuehappy has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-11-03T16:42:25Z

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

java/src/main/java/org/rocksdb/Checkpoint.java

facebook-github-bot · 2023-11-04T03:29:33Z

@mayuehappy has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-11-06T02:54:46Z

@mayuehappy has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-11-06T02:55:58Z

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

facebook-github-bot · 2023-11-06T15:44:13Z

@cbi42 merged this pull request in 19768a9.

Summary: - Add the following missing options to src/main/java/org/rocksdb/ImportColumnFamilyOptions.java and in java/rocksjni/import_column_family_options.cc in RocksJava. - Add the struct to src/main/java/org/rocksdb/ExportImportFilesMetaData.java and in java/rocksjni/export_import_files_metadatajni.cc in RocksJava. - Add New Java API `createColumnFamilyWithImport` to src/main/java/org/rocksdb/RocksDB.java - Add New Java API `exportColumnFamily` to src/main/java/org/rocksdb/Checkpoint.java Pull Request resolved: facebook/rocksdb#11646 Test Plan: - added unit tests for exportColumnFamily in org.rocksdb.CheckpointTest - added unit tests for createColumnFamilyWithImport to org.rocksdb.ImportColumnFamilyTest Reviewed By: ajkr Differential Revision: D50889700 Pulled By: cbi42 fbshipit-source-id: d623b35e445bba62a0d3c007d74352e937678f6c

facebook-github-bot added the CLA Signed label Jul 27, 2023

mayuehappy force-pushed the jni-support2 branch 2 times, most recently from 24a4d37 to 70bd9af Compare July 27, 2023 13:48

adamretter self-requested a review July 28, 2023 14:34

adamretter requested changes Jul 28, 2023

View reviewed changes

StefanRRichter mentioned this pull request Sep 7, 2023

[FLINK-31238] [WIP] Use IngestDB to speed up Rocksdb rescaling recovery apache/flink#23169

Open

adamretter reviewed Oct 6, 2023

View reviewed changes

mayuehappy added 4 commits October 8, 2023 15:03

add jni support for CreateColumnFamilyWithImport

8dcc9a5

fix comment

ee327e0

fix comment

cf810a4

fix comment

57e2b16

mayuehappy force-pushed the jni-support2 branch from 16fc045 to 57e2b16 Compare October 8, 2023 07:03

alanpaxton reviewed Nov 1, 2023

View reviewed changes

address comments

710fd7c

cbi42 requested review from alanpaxton and adamretter November 2, 2023 16:20

alanpaxton reviewed Nov 2, 2023

View reviewed changes

java/src/main/java/org/rocksdb/ExportImportFilesMetaData.java Outdated Show resolved Hide resolved

resolve comment

bf29361

Merge branch 'main' into jni-support2

db02ebe

cbi42 reviewed Nov 3, 2023

View reviewed changes

java/src/main/java/org/rocksdb/Checkpoint.java Outdated Show resolved Hide resolved

resolve 'unused import'

b8b6783

Merge branch 'main' into jni-support2

4520b64

facebook-github-bot closed this in 19768a9 Nov 6, 2023

facebook-github-bot added the Merged label Nov 6, 2023

This was referenced Nov 21, 2023

Pre-work of adding ExportColumnFamily and CreateColumnFamilyWithImport for Java #6894

Closed

Add ExportColumnFamily for Java #6895

Closed

igorcanadi mentioned this pull request Jan 17, 2024

[SYS-6913] Upgrade RocksDB-Cloud to 8.9.1 rockset/rocksdb-cloud#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jni Support for API CreateColumnFamilyWithImport #11646

Add jni Support for API CreateColumnFamilyWithImport #11646

mayuehappy commented Jul 27, 2023 •

edited by cbi42

Loading

adamretter left a comment

adamretter Jul 28, 2023

adamretter Jul 28, 2023

adamretter Jul 28, 2023 •

edited

Loading

mayuehappy Jul 30, 2023 •

edited

Loading

adamretter Sep 8, 2023

mayuehappy commented Sep 8, 2023

adamretter commented Sep 8, 2023

mayuehappy commented Sep 27, 2023

adamretter Oct 6, 2023

mayuehappy Oct 8, 2023

pnowojski commented Oct 20, 2023

mayuehappy commented Oct 23, 2023

adamretter commented Nov 1, 2023

alanpaxton left a comment

alanpaxton Nov 1, 2023

alanpaxton Nov 1, 2023

alanpaxton Nov 1, 2023

facebook-github-bot commented Nov 1, 2023

cbi42 commented Nov 1, 2023

facebook-github-bot commented Nov 2, 2023

mayuehappy commented Nov 2, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 4, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

Add jni Support for API CreateColumnFamilyWithImport #11646

Add jni Support for API CreateColumnFamilyWithImport #11646

Conversation

mayuehappy commented Jul 27, 2023 • edited by cbi42 Loading

adamretter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

mayuehappy Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayuehappy commented Sep 8, 2023

adamretter commented Sep 8, 2023

mayuehappy commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski commented Oct 20, 2023

mayuehappy commented Oct 23, 2023

adamretter commented Nov 1, 2023

alanpaxton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 1, 2023

cbi42 commented Nov 1, 2023

facebook-github-bot commented Nov 2, 2023

mayuehappy commented Nov 2, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 4, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

facebook-github-bot commented Nov 6, 2023

mayuehappy commented Jul 27, 2023 •

edited by cbi42

Loading

adamretter Jul 28, 2023 •

edited

Loading

mayuehappy Jul 30, 2023 •

edited

Loading