-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-6210] close RocksDB in ListViaMergeSpeedMiniBenchmark && ListV… #3652
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
Conversation
…iaRangeSpeedMiniBenchmark
shixiaogang
left a comment
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.
@fanyon Thanks a lot for the opened PR. These changes help to enhance the safety of the benchmarks. But besides the RocksDB, some other objects should be closed as well when the program exits. You can take a look at the comments for the details.
|
|
||
| final String key = "key"; | ||
| final String value = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ7890654321"; | ||
| try { |
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.
I think it's better to use try-with-resources here.
| System.out.println("end get - duration: " + ((endGet3 - beginGet3) / 1_000_000) + " ms"); | ||
| System.out.println("end get - duration: " + ((endGet3 - beginGet3) / 1_000_000) + " ms"); | ||
| } finally { | ||
| rocksDB.close(); |
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.
Besides RocksDB, Options andWriteOptions should also be closed here.
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.
I think it's better to delete the RocksDB's directory here.
|
|
||
| final long beginGet = System.nanoTime(); | ||
|
|
||
| final RocksIterator iterator = rocksDB.newIterator(); |
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.
The iterator should be closed once it's not used. So it's better to use try-with-resources here.
| System.out.println("end get - duration: " + ((endGet - beginGet) / 1_000_000) + " ms"); | ||
| System.out.println("end get - duration: " + ((endGet - beginGet) / 1_000_000) + " ms"); | ||
| } finally { | ||
| rocksDB.close(); |
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.
As with the problem mentioned above, all RocksDB objects should be closed once they are not used.
|
Hi @shixiaogang , thank you for your reviews. Close and clean all resources such as temp directories is a good idea, I'll fix them later in this PR |
2. remove rocksdb tmp directory
|
Hi @shixiaogang , as we discussed before, I have closed all resources in latest commit, thank you for your comments. |
|
Thank you for the patch! |
…k && ListViaRangeSpeedMiniBenchmark This closes apache#3652
…k && ListViaRangeSpeedMiniBenchmark This closes apache#3652
…k && ListViaRangeSpeedMiniBenchmark This closes apache#3652
…k && ListViaRangeSpeedMiniBenchmark This closes apache#3652
close RocksDB in ListViaMergeSpeedMiniBenchmark && ListViaRangeSpeedMiniBenchmark