Simplify a test case in Java ReadOnlyTest #7608
Conversation
ghost
requested review from
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Cheng-Chang @zhichao-cao I am afraid that there are problems with this PR. Several |
There was a problem hiding this comment.
This PR has problems. Objects that were previously free'd no longer are. Please see the documentation about this here - https://github.com/facebook/rocksdb/wiki/RocksJava-Basics#memory-management
Thanks for pointing out. I did not realize the issue. |
@mrambacher Thanks for pointing out the link, I didn't realize every object needs to be closed manually. The db instances and column family handles are closed. I'm wondering is it necessary to close the options and descriptors objects manually? I checked the implementation, relying on GC for options and descriptors looks good to me. |
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
| assertThat("value").isEqualTo(new String(db.get("key".getBytes()))); | ||
| } | ||
|
|
||
| final ColumnFamilyOptions cfOpts = new ColumnFamilyOptions(); |
There was a problem hiding this comment.
It's weird to close an option object, but I understand the motivation behind this to free the memory asap.
But in the codebase, options are not explicitly closed everywhere, e.g. https://github.com/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/ColumnFamilyDescriptor.java#L24.
| assertThat(new String(db.get(readOnlyColumnFamilyHandleList2.get(1), "key2".getBytes()))) | ||
| .isEqualTo("value2"); | ||
| } finally { | ||
| for (ColumnFamilyHandle handle : readOnlyColumnFamilyHandleList2) { |
There was a problem hiding this comment.
In the latest codebase. Any column family handles created by a RocksDB#open call, will be auto-magically cleaned up in the corresponding RocksDB#close call.
So you may be able to benefit from that, and tidy this up further.
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
2 similar comments
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Cheng-Chang merged this pull request in 1f62721. |
Summary: The original test nests a lot of `try` blocks. This PR flattens these blocks into independent blocks, so that each `try` block closes the DB before opening the next DB instance. Pull Request resolved: facebook#7608 Test Plan: watch the existing java tests to pass Reviewed By: zhichao-cao Differential Revision: D24611621 Pulled By: cheng-chang fbshipit-source-id: d486c5d37ac25d4b860d739ef2cdd58e6064d42d
### What changes are proposed in this pull request? Fixes facebook/rocksdb#9378 The `orgs.rocksdb.RocksObject` extends `AutoCloseable` so they should be closed properly to avoid leaks. ### Why are the changes needed? According to the [RocksDB documentation](https://github.com/facebook/rocksdb/wiki/RocksJava-Basics#memory-management) and a sample [PR discussion](facebook/rocksdb#7608 (comment)), the objects need to be closed or wrapped in a try-with-resource block. ### Does this PR introduce any user facing changes? NA pr-link: #14856 change-id: cid-69a164c547a14e0bb7710fc7a48731aecba4db88
### What changes are proposed in this pull request? Fixes facebook/rocksdb#9378 The `orgs.rocksdb.RocksObject` extends `AutoCloseable` so they should be closed properly to avoid leaks. ### Why are the changes needed? According to the [RocksDB documentation](https://github.com/facebook/rocksdb/wiki/RocksJava-Basics#memory-management) and a sample [PR discussion](facebook/rocksdb#7608 (comment)), the objects need to be closed or wrapped in a try-with-resource block. ### Does this PR introduce any user facing changes? NA pr-link: #14856 change-id: cid-69a164c547a14e0bb7710fc7a48731aecba4db88
Close RocksDB objects that are AutoCloseable
### What changes are proposed in this pull request?
Fixes facebook/rocksdb#9378
The `orgs.rocksdb.RocksObject` extends `AutoCloseable` so they should be
closed properly to avoid leaks.
### Why are the changes needed?
According to the [RocksDB
documentation](https://github.com/facebook/rocksdb/wiki/RocksJava-Basics#memory-management)
and a sample [PR
discussion](facebook/rocksdb#7608 (comment)),
the objects need to be closed or wrapped in a try-with-resource block.
### Does this PR introduce any user facing changes?
NA
pr-link: Alluxio#14856
change-id: cid-69a164c547a14e0bb7710fc7a48731aecba4db88
The original test nests a lot of
tryblocks. This PR flattens these blocks into independent blocks, so that eachtryblock closes the DB before opening the next DB instance.Test Plan:
watch the existing java tests to pass