-
Notifications
You must be signed in to change notification settings - Fork 40
Add more waits for cache expiry #3046
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
|
/gemini review |
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.
Code Review
This pull request addresses flaky integration tests caused by metadata cache staleness. The changes introduce two strategies: adding explicit waits for cache expiry and refactoring tests to use unique table names to ensure isolation.
The refactoring to use unique table names in DistributedTransactionAdminIntegrationTestBase is a great improvement. It makes the tests more robust and isolated, and the code is cleaner with the improved use of try-with-resources.
The addition of sleepUninterruptibly(1, TimeUnit.SECONDS) is a pragmatic fix for the caching issue. However, the sleep duration 1 is a magic number. I've left comments suggesting to extract this value into a shared constant in the DistributedTransactionAdminIntegrationTestBase class to improve readability and maintainability.
Overall, the changes are good and should help improve the stability of the test suite.
...ntegration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdminIntegrationTest.java
Show resolved
Hide resolved
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Show resolved
Hide resolved
.../java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdminIntegrationTestBase.java
Show resolved
Hide resolved
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.
Pull Request Overview
This PR addresses test failure issues caused by stale metadata cache in integration tests by adding cache expiry waits and using unique table names for DML operations.
- Adds 1-second waits before transactional insert and scan operations to allow cache expiry
- Updates test methods to use unique table names instead of shared ones to avoid stale cache conflicts
- Refactors test cases to use try-with-resources for better resource management
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SingleCrudOperationTransactionAdminIntegrationTestBase.java | Added cache expiry waits in transactional operations |
| ConsensusCommitAdminIntegrationTestBase.java | Added cache expiry waits in transactional operations |
| DistributedTransactionAdminIntegrationTestBase.java | Used unique table names and improved resource management in DML test cases |
| JdbcTransactionAdminIntegrationTest.java | Added cache expiry waits in transactional operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Show resolved
Hide resolved
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Show resolved
Hide resolved
brfrn169
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.
LGTM! Thank you!
|
To resolve the following test failure due to the Cassandra stale cache issue, I revised the code to use a separate transaction manager instance for each DML operation in f3f5726. |
feeblefakie
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.
LGTM! Thank you!
Description
This PR adds more waits for cache expiry in integration test cases for administrative operations. The purpose is to resolve the test failure issue caused by stale metadata cache on Cluster side.
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A