-
Notifications
You must be signed in to change notification settings - Fork 509
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-10783. Close SstFileReaderIterator in RocksDBCheckpointDiffer #6616
Conversation
…cksDBCheckpointDiffer
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.
Thanks for the patch @hemantk-12 LGTM
Thanks @swamirishi for the quick review. |
Is SstFileReader and the other RocksDB classes listed in this block supposed to be allowed imports for the checkpoint differ? It looks like direct use of this class instead of the managed version is what led to this bug. |
@errose28 you are right, direct use of the RocksDB classes led to this bug. We should be able to remove most of the RocksDB's direct usage except RocksDB objects used in here. Created follow up task: https://issues.apache.org/jira/browse/HDDS-10787 |
Filed HDDS-10008 some time ago to clean up the allowed imports (triggered by another leak, of |
…pache#6616) (cherry picked from commit a658802)
…intDiffer (apache#6616) Change-Id: I6d81cbd3a872b1598aa211949bcb42a3c91b8fdf
…pache#6616) (cherry picked from commit a658802)
…pache#6616) (cherry picked from commit a658802)
…pache#6616) (cherry picked from commit a658802)
…pache#6616) (cherry picked from commit a658802)
…pache#6616) (cherry picked from commit a658802)
What changes were proposed in this pull request?
SstFileReaderIterator is not getting closed in RocksDBCheckpointDiffer that could potentially leave the file descriptor open even after RocksDB compaction deletes the file. This change is to close the SstFileReaderIterator after the use.
What is the link to the Apache JIRA
HDDS-10783
How was this patch tested?
Existing tests.