HDDS-14800. Guard RocksDB iterator against closed DB during volume#25
HDDS-14800. Guard RocksDB iterator against closed DB during volume#25priyeshkaratha wants to merge 1 commit intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical race condition where RocksDB iterators could attempt to access a database that has been concurrently closed, leading to native crashes. The changes introduce robust checks within the iterator logic to determine if the database is closed before proceeding with operations, allowing for a clean exit. Furthermore, the database's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism to prevent native crashes when a RocksDB instance is closed while iterators are still active. This is achieved through a combination of reference counting to delay the physical closing of the database and adding guards to iterator methods to handle the logically closed state gracefully. The implementation is solid and is accompanied by a comprehensive set of new unit and integration tests that simulate the race condition. The changes significantly improve the stability of database interactions. I found one minor typo in an exception message that I've commented on.
| throw new RocksDatabaseException("Cannot removeFromDB: underlying RocksDB for table " | ||
| + rocksDBTable.getName() + "is closed"); |
There was a problem hiding this comment.
There's a minor typo in the exception message string. A space is missing before "is closed", which will result in a message like "...tableNameis closed". Adding a space will improve readability.
| throw new RocksDatabaseException("Cannot removeFromDB: underlying RocksDB for table " | |
| + rocksDBTable.getName() + "is closed"); | |
| throw new RocksDatabaseException("Cannot removeFromDB: underlying RocksDB for table " | |
| + rocksDBTable.getName() + " is closed"); |
…ilure
What changes were proposed in this pull request?
Provide a one-liner summary of the changes in the PR Title field above.
It should be in the form of
HDDS-1234. Short summary of the change.Please describe your PR in detail:
perspective not just for the reviewer.
the Jira's description if the jira is well defined.
issue investigation, github discussion, etc.
Examples of well-written pull requests:
What is the link to the Apache JIRA
Please create an issue in ASF JIRA before opening a pull request, and you need to set the title of the pull
request which starts with the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
If you do not have an ASF Jira account yet, please follow the first-time contributor
instructions in the Jira guideline.
(Please replace this section with the link to the Apache JIRA)
How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests, workflow run on the fork git repo.)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this.)