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
Abstract merge operator in Java + Thread JNI attachment automatic management + loader management for static variables #3432
base: main
Are you sure you want to change the base?
Conversation
…ads+ loaders/unloaders for static vars
…ads+ loaders/unloaders for static vars
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
In the branch it is present also my transaction patch. Eventually if adamitter commited his transaction branch , i can merge( or remove mime) so we are aligned with master branch. The current patch is contained just in commits |
@publicocean0 has updated the pull request. View: changes |
@publicocean0 has updated the pull request. View: changes |
@publicocean0 has updated the pull request. View: changes |
…act calls. Rockdb secondary threads are are not joined correctly
@publicocean0 has updated the pull request. View: changes |
Found a probable bug in rocksdb when db is closing: threads not joined in synchronous way with delete db instruction. #3453. i dont find any way for waiting the rocksdb threads are terminated after db deletion. |
@publicocean0 has updated the pull request. View: changes |
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.
Automatic thread attach / detach would be super helpful, it's something I ran into with #3338 and was hoping for solutions.
@ajkr mentioned they are working on some systemic approach to solve this, I am not sure what stage that effort is in.
@publicocean0 my personal preference would be to split the attachment management from the abstract merge operator to reduce size of the PRs. Then there's no example of using the automatic thread attach / detach but I think would make this easier to reviewers to parse.
@@ -27,7 +27,7 @@ make_config.mk | |||
CMakeCache.txt | |||
CMakeFiles/ | |||
build/ | |||
|
|||
cmake-build-debug/ |
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.
Needed? Looks like it got included anyway.
@@ -930,7 +930,7 @@ if(WITH_TESTS) | |||
table/mock_table.cc | |||
util/fault_injection_test_env.cc | |||
utilities/cassandra/test_utils.cc | |||
) | |||
java/rocksjni/init.h java/rocksjni/init.cc) |
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.
Sort
@@ -0,0 +1,242 @@ | |||
# CMAKE generated file: DO NOT EDIT! |
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.
Remove this entire directory, it's generated
@@ -66,6 +66,7 @@ | |||
#define EXT4_SUPER_MAGIC 0xEF53 | |||
#endif | |||
|
|||
|
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.
Undo all whitespace changes
@@ -54,7 +54,10 @@ NATIVE_JAVA_CLASSES = org.rocksdb.AbstractCompactionFilter\ | |||
org.rocksdb.WriteBatch.Handler\ | |||
org.rocksdb.WriteOptions\ | |||
org.rocksdb.WriteBatchWithIndex\ | |||
org.rocksdb.WBWIRocksIterator | |||
org.rocksdb.WBWIRocksIterator\ | |||
org.rocksdb.AbstractAssociativeMergeOperator\ |
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.
Sort, fix indent
RocksDB.loadLibrary(); | ||
} | ||
|
||
|
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.
Undo whitespace changes
} | ||
StringBuffer sb=new StringBuffer(new String(oldvalue)); | ||
sb.append(','); | ||
//if (1==1)throw new IndexOutOfBoundsException(); |
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.
Remove debug / commented lines in this file
@@ -209,6 +210,197 @@ public void merge() throws RocksDBException { | |||
"xxxx".getBytes()); | |||
} | |||
} | |||
private class M extends AbstractAssociativeMergeOperator{ |
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.
Can you move this and the associated tests to its own test file?
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.
Ok tests works but there is a problem to solve for having the best performance. The performance is anyway better using initialization strategy because it preloads all what is possible(jvm variables handles loading consumes cpu).
Threads lanched by rocksdb are not terminated in synchronous way with "delete db"(pratically rocks threads are terminated after (when process is closing )so it happens jvm is closed before rocksdb threads are closed creating a memory leak. For evoiding it for now i create context as before (create-destroy cycle) but in future i will remove it solving this problem. This problem appear just with rocksdb threads not with java threads or c++ threads created on the fly if you use join in the end. ).
For solving the problem i might add a simple method in posix-env joining all threads inside thread pool.
} | ||
} | ||
|
||
//@Test |
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.
?
@@ -373,6 +373,7 @@ MAIN_SOURCES = \ | |||
utilities/write_batch_with_index/write_batch_with_index_test.cc \ | |||
|
|||
JNI_NATIVE_SOURCES = \ | |||
java/rocksjni/init.cc \ |
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.
Sort
Closing it since there was no interest shown to have it merged. |
@maysamyabandeh Somehow I missed this. I will reopen it if you don't mind as it sounds like there are some useful additions in this branch such as the thread attach/detacth |
@adamretter any update on this? |
@publicocean0 Okay I pushed a commit to your branch to get it to compile in its current state on MacOS. I also created a branch where I rebased it onto HEAD and stripped out the transaction stuff from the history - https://github.com/adamretter/rocksdb/tree/abstractMergeOperator-REBASED However, when I run the testsuite on your branch via:
I get a SIGSEGV in your new merge operator code:
The full-stack trace from the hs_err log file then looks like:
|
Related to #2282 |
This pull request add AbstractAssociativeMergeOperator and AbstractNotAssociativeMergeOperator in java. These classes are abstract permitting to implement the merge operator in java code.
This pull request introduces also 2 important performance improvements(they are generic applicable to all jni classes):
The thread attachment management and loading management are used by merge operator for optimizing the execution time