-
Notifications
You must be signed in to change notification settings - Fork 491
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
HDDS-7087. Manage RocksObjects to detect leaks #3656
Conversation
I like this approach; @jojochuang mentioned that there might be ways in the pom for all packages other than managed packaged to block rocks DB import. |
There's a Maven plugin to ban certain classpath: https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html |
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.
@DuongNguyen0 , thanks a lot for working on this! The idea of having managed objects is great. We should remove the import of org.rockdb.* from the other classes in order to avoid missing the changes. See also the comments inlined.
...rvice/src/main/java/org/apache/hadoop/ozone/container/common/utils/db/DatanodeDBProfile.java
Outdated
Show resolved
Hide resolved
...-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBConfigFromFile.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBProfile.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBProfile.java
Outdated
Show resolved
Hide resolved
...ds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksIterator.java
Outdated
Show resolved
Hide resolved
Thanks @kerneltime, @jojochuang and @szetszwo for the early review and suggestions. I've updated the code to (mostly) ban the usage of |
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.
+1 the change looks good.
Just a minor comment inlined.
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java
Outdated
Show resolved
Hide resolved
Why the checks were skipped? |
Because I was a draft PR. Now all the checks are running. |
What changes were proposed in this pull request?
We need a mechanism to detect if any RocksObject is not closed before being GCed.
This PR replaces the usage of RocksObject classes in
org.rocksdb.*
package by managed RocksObject classes whose finalizers are intercepted to assert of their instances are closed before GCed.https://issues.apache.org/jira/browse/HDDS-7087
Sample data
Metric showing memory leaks.
Sample error message when RocksObject is used in Ozone.
How was this patch tested?
Standard CI: https://github.com/duongnguyen0/ozone/actions/runs/2805369172