-
Notifications
You must be signed in to change notification settings - Fork 41
Fix the integration test and workflow for Object Storage adapter #3232
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
...ava/com/scalar/db/storage/objectstorage/ConsensusCommitIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
...ava/com/scalar/db/storage/objectstorage/ObjectStorageConditionalMutationIntegrationTest.java
Show resolved
Hide resolved
...db/storage/objectstorage/SingleCrudOperationTransactionIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
|
/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 introduces fixes for integration tests on S3 and Cloud Storage by working around their rate limits. The changes involve adding waits in write-heavy tests and modifying the test workflow. The approach is sound, but there are opportunities to improve maintainability by reducing code duplication and eliminating magic numbers. I've also identified a potential issue where a thread-limiting override was removed, which might reintroduce the very rate-limiting problems this PR aims to solve.
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 integration test failures for S3 and Cloud Storage adapters caused by strict request rate limits. The solution splits tests into separate buckets per test group and adds rate-limiting waits for Cloud Storage's strict 1-write-per-second limit.
Key changes:
- Expanded the GitHub Actions workflow matrix from 2 test modes to 14 distinct test groups, each using a separate S3/Cloud Storage bucket to avoid rate limit conflicts
- Added Cloud Storage-specific sleeps (1 second per write operation, 2 seconds in setUp/tearDown) to handle the strict rate limits
- Made visibility changes to the base test class to enable subclass customization for Cloud Storage
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/object-storage-adapter-check.yaml |
Reorganized test execution into 14 distinct test groups, each using separate buckets with descriptive suffixes; updated S3 region and bucket naming |
ConsensusCommitSpecificIntegrationTestBase.java |
Changed visibility of fields (recoveryExecutor, groupCommitter) and methods (setUp, truncateTables) from private to protected to support subclass overrides |
ConsensusCommitSpecificIntegrationTestWithObjectStorage.java |
Added Cloud Storage-specific sleeps in setUp/tearDown methods and duplicated cleanup logic from parent |
ConsensusCommitIntegrationTestWithObjectStorage.java |
Added Cloud Storage-specific sleeps in setUp/tearDown methods |
SingleCrudOperationTransactionIntegrationTestWithObjectStorage.java |
Overrode populateRecords() to add 1-second sleeps between writes for Cloud Storage rate limiting |
ObjectStorageSinglePartitionKeyIntegrationTest.java |
Added getColumnWithMaxValue() override to use 'Z' characters instead of 0xFF for Cloud Storage TEXT max values |
ObjectStorageSingleClusteringKeyScanIntegrationTest.java |
Added getColumnWithMaxValue() override to use 'Z' characters instead of 0xFF for Cloud Storage TEXT max values |
ObjectStorageMultiplePartitionKeyIntegrationTest.java |
Added getColumnWithMaxValue() override to use 'Z' characters instead of 0xFF for Cloud Storage TEXT max values |
ObjectStorageMultipleClusteringKeyScanIntegrationTest.java |
Added getColumnWithMaxValue() override to use 'Z' characters instead of 0xFF for Cloud Storage TEXT max values |
ObjectStorageConditionalMutationIntegrationTest.java |
Removed custom thread count override (reverted to base class default) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...java/com/scalar/db/storage/objectstorage/ObjectStorageSinglePartitionKeyIntegrationTest.java
Show resolved
Hide resolved
...scalar/db/storage/objectstorage/ConsensusCommitSpecificIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
...scalar/db/storage/objectstorage/ConsensusCommitSpecificIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
...ava/com/scalar/db/storage/objectstorage/ConsensusCommitIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
...scalar/db/storage/objectstorage/ConsensusCommitSpecificIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
...ava/com/scalar/db/storage/objectstorage/ConsensusCommitIntegrationTestWithObjectStorage.java
Show resolved
Hide resolved
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! 👍
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!
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!
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 fixes the integration test and workflow for S3 and Cloud Storage adapter.
Since S3 and Cloud Storage have a strict request rate limit, integration tests for those adapters fail, currently. To address this issue, this PR fixes the test workflow to execute those tests in separate buckets.
Additionally, since Cloud Storage has a stricter request rate limit allowing one write per second, this PR introduces some waits for some write-heavy spots.
The test is verified in https://github.com/scalar-labs/scalardb/actions/runs/19738491550.
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A