-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-14210. TestOmDBInsightEndPoint may fail. #9629
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
base: master
Are you sure you want to change the base?
Conversation
adoroszlai
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.
Thanks @ArafatKhan2198 for working on this. If it's a test problem, please fix it in the test. We should not ignore closed database globally.
I agree with you @adoroszlai it's better to fix the test. |
| ozoneStorageContainerManager.stop(); | ||
| } | ||
| if (reconDBProvider != null) { | ||
| reconDBProvider.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.
Thanks @ArafatKhan2198 for updating the patch.
ReconTestInjector is created withContainerDB in @BeforeEach of some other tests. Don't they need to close it?
Examples:
- TestClusterStateEndpoint
- TestDeletedKeysSearchEndpoint
- TestOpenKeysSearchEndpoint
(There may be more.)
Also, it would be better to add ReconTestInjector.close() to clean up all resources it created, and let tests call that single method instead of storing and closing reconDBProvider, which is otherwise not directly used in the test.
|
Thanks for the review comment @adoroszlai Based on my findings, here is the list of test classes that initialise ReconTestInjector with .withContainerDB() (which creates the RocksDB instance) but are missing the cleanup logic. These 13 files need the same fix: TestClusterStateEndpoint.java Would you suggest that we fix for TestClusterStateEndpoint only in this jira? |
|
I think this change can wait for a proper fix. I have not seen this test fail on On the other hand, |
| reconDBProvider.close(); | ||
| } | ||
| } catch (Exception e) { | ||
| } |
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.
in case of exception here we should fail the test.
What changes were proposed in this pull request?
This PR fixes a flaky test failure in
TestOmDBInsightEndPoint, where tests would randomly fail withRocksDatabaseException: Rocks Database is closed.The issue was that
ReconDBProviderandOzoneStorageContainerManagerwere created for each test but were never properly cleaned up. When a test finished, these resources remained open, leaving the database files locked. As a result, the next test could fail.The fix includes:
OzoneStorageContainerManagerandReconDBProvidertearDown()to properly stop and close these resources after each testThis ensures all database connections are closed before the next test runs, preventing the “database is closed” error.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14210
How was this patch tested?
Ran the test 100 times, and passed in all of them - https://github.com/ArafatKhan2198/ozone/actions/runs/21200116059/job/60984087538#logs