Refactor ColumnFamilyDescriptor to be RocksObject #12505
Conversation
rhubner
force-pushed
the
eb/cf-descriptor-refactor
branch
2 times, most recently
from
9a2c8af
to
624b2ae
Compare
| (JNIEnv* env, jclass, jlong jcf_descriptor_handle) { | ||
| auto cf_options = reinterpret_cast<ROCKSDB_NAMESPACE::ColumnFamilyDescriptor*>(jcf_descriptor_handle); | ||
|
|
||
| return GET_CPLUSPLUS_POINTER(&cf_options->options); //TODO -> Should we copy options and leave it on Java developer to delete? |
There was a problem hiding this comment.
I need to just check through some ownership stuff here, and then I will come back to you on this...
| @@ -6779,14 +6779,14 @@ class ColumnFamilyDescriptorJni : public JavaClass { | |||
| } | |||
|
|
|||
| jmethodID mid = env->GetMethodID(jclazz, "<init>", | |||
| "([BLorg/rocksdb/ColumnFamilyOptions;)V"); | |||
| "([BLorg/rocksdb/ColumnFamilyOptions;Z)V"); | |||
There was a problem hiding this comment.
Do we still create Java ColumnFamilyDescriptor objects from C++, I would think not after this refactoring...
There was a problem hiding this comment.
This is where I still have doubts. This code is used primarily to load options from file. We can approach creation of ColumnFamilyDescriptor two ways :
- Just wrap the pointer. But then it breaks all existing code with ColumnFamilyOptions.
- Create new instances with ColumnFamilyOptions and keep ColumnFamilyDescriptor behavior close to what we already have in the codebase.
There was a problem hiding this comment.
Okay let me look into this a bit deeper and come back to you...
java/rocksjni/rocksjni.cc
Outdated
| jcf_options_handles_elems, JNI_ABORT); | ||
| return nullptr; | ||
| } | ||
| env->GetLongArrayRegion(jcf_descriptor_handles, 0, jlen, cf_descriptor_handles.get()); |
There was a problem hiding this comment.
We need to check for an exception after this JNI call
| } | ||
| @Override | ||
| protected void disposeInternal(final long handle) { | ||
| disposeJni(nativeHandle_); |
There was a problem hiding this comment.
I think the order of these functions calls should be swapped, and the disposeJni should go in a finally block.
There was a problem hiding this comment.
How about instead, so that we don't for this class have a native method called disposeJni instead of the usual name: disposeInternal:
@Override
protected void disposeInternal() {
try {
if (implicitlyCreatedColumnFamilyOptions) {
columnFamilyOptions.close();
}
} finally {
super.disposeInternal();
}
}There was a problem hiding this comment.
disposeJni is static. And I can't override method with static method. This is why I have disposeJni
I will swap the order and add finally block.
There was a problem hiding this comment.
Okay but you don't need to add a disposeJni method. Other classes that extend RocksObject add a disposeInternal(nativeHandle) method - we should follow the same pattern here.
|
|
||
| assertThat(result).isNotNull(); | ||
|
|
||
| ColumnFamilyOptions cfOptions = result.getOptions(); |
| try (final RocksDB db = | ||
| RocksDB.open(dbFolder.getRoot().getAbsolutePath()); | ||
| ColumnFamilyDescriptor cfDescriptor = new ColumnFamilyDescriptor("myCf".getBytes(StandardCharsets.UTF_8))) { | ||
| ColumnFamilyHandle cfHandle = db.createColumnFamily(cfDescriptor); |
|
|
||
| try (final RocksDB db = | ||
| RocksDB.open(dbFolder.getRoot().getAbsolutePath()); | ||
| ColumnFamilyDescriptor cfDescriptor = new ColumnFamilyDescriptor("myCf".getBytes(StandardCharsets.UTF_8))) { |
| ColumnFamilyDescriptor cfDescriptor = new ColumnFamilyDescriptor("myCf".getBytes(StandardCharsets.UTF_8))) { | ||
| ColumnFamilyHandle cfHandle = db.createColumnFamily(cfDescriptor); | ||
|
|
||
| ColumnFamilyDescriptor result = cfHandle.getDescriptor(); // This creates new ColumnFamilyDescriptor in JNI code. |
|
|
||
| @Test | ||
| public void closeTransitiveResources() throws RocksDBException { | ||
| ColumnFamilyDescriptor cfDescriptor = new ColumnFamilyDescriptor("myCf".getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
final variables please (also more below!)
|
@rhubner Do you also need to update the |
src.mk
Outdated
| @@ -651,6 +651,7 @@ JNI_NATIVE_SOURCES = \ | |||
| java/rocksjni/checkpoint.cc \ | |||
| java/rocksjni/clock_cache.cc \ | |||
| java/rocksjni/cache.cc \ | |||
| java/rocksjni/columncamilydescriptor.cc \ | |||
There was a problem hiding this comment.
columncamilydescriptor.cc -> columnfamilydescriptor.cc
There was a problem hiding this comment.
Just wondering if this indicates that tests have not been run yet with the Make build?
There was a problem hiding this comment.
I think I name the file wrongly and then I copied error everywhere.
Test are running : https://github.com/facebook/rocksdb/actions/runs/8565557203/job/23473783606?pr=12505
rhubner
force-pushed
the
eb/cf-descriptor-refactor
branch
from
624b2ae
to
5cc529a
Compare
Fix memory leak, see #12224 for details