-
Notifications
You must be signed in to change notification settings - Fork 40
Add wait for cache expiry #3040
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 is a great pull request that addresses the flakiness in integration tests by waiting for cache expiry. The approach of shortening the cache expiration time and adding a wait is a practical solution for the problem of stale metadata in a clustered environment. The refactoring in the test methods to use a single try-with-resources block is also a nice improvement that simplifies the code. I have a couple of suggestions to make the tests even more robust and the code more maintainable.
...est/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminIntegrationTest.java
Show resolved
Hide resolved
...alar/db/storage/jdbc/SingleCrudOperationTransactionAdminIntegrationTestWithJdbcDatabase.java
Show resolved
Hide resolved
...ntegration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdminIntegrationTest.java
Show resolved
Hide resolved
...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java
Show resolved
Hide resolved
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraEnv.java
Show resolved
Hide resolved
...est/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java
Show resolved
Hide resolved
...est/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraEnv.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 failures caused by stale table metadata in administrative operations by setting short cache expiration times and adding explicit waits for cache expiry in integration tests.
- Sets metadata cache expiration time to 1 second across all Core integration test environments
- Adds 1-second sleep waits in specific test methods after column type alterations to allow cache expiry
- Refactors resource management to use try-with-resources pattern for proper cleanup
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| DistributedTransactionAdminIntegrationTestBase.java | Adds cache expiry wait and refactors to try-with-resources |
| DistributedStorageAdminIntegrationTestBase.java | Adds cache expiry wait and refactors to try-with-resources |
| JdbcTransactionAdminIntegrationTest.java | Adds cache expiry wait and refactors to try-with-resources |
| SingleCrudOperationTransactionAdminIntegrationTestWithJdbcDatabase.java | Adds cache expiry wait and refactors to try-with-resources |
| JdbcAdminIntegrationTest.java | Adds cache expiry wait and refactors to try-with-resources |
| ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java | Adds cache expiry wait and refactors to try-with-resources |
| CassandraEnv.java | Sets metadata cache expiration time to 1 second |
| CosmosEnv.java | Sets metadata cache expiration time to 1 second |
| DynamoEnv.java | Sets metadata cache expiration time to 1 second |
| JdbcEnv.java | Sets metadata cache expiration time to 1 second |
| MultiStorageEnv.java | Sets metadata cache expiration time to 1 second |
| MultiStorageAdminIntegrationTest.java | Sets metadata cache expiration time to 1 second |
| MultiStorageIntegrationTest.java | Sets metadata cache expiration time to 1 second |
| MultiStorageMutationAtomicityUnitIntegrationTest.java | Sets metadata cache expiration time to 1 second |
| MultiStorageSchemaLoaderIntegrationTest.java | Sets metadata cache expiration time to 1 second |
| ConsensusCommitSpecificIntegrationTestWithMultiStorage.java | Sets metadata cache expiration time to 1 second |
| ConsensusCommitNullMetadataIntegrationTestWithMultiStorage.java | Sets metadata cache expiration time to 1 second |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ntegration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdminIntegrationTest.java
Show resolved
Hide resolved
...alar/db/storage/jdbc/SingleCrudOperationTransactionAdminIntegrationTestWithJdbcDatabase.java
Show resolved
Hide resolved
...est/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.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!
komamitsu
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! 👍
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!
Torch3333
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 waits for cache expiry in integration test cases for administrative operations.
Currently, some admin test cases fail due to stale table metadata. And it is difficult to invalidate them, especially in a ScalarDB Cluster.
I and @brfrn169 discussed it, and decided to make the expiration time short and insert waits for expiry.
Related issues and/or PRs
Changes made
alterColumnType_WideningConversion_ShouldAlterColumnTypesCorrectlyalterColumnType_Oracle_WideningConversion_ShouldAlterColumnTypesCorrectlyChecklist
Additional notes (optional)
N/A
Release notes
N/A