-
Notifications
You must be signed in to change notification settings - Fork 41
Fix option issues in Object Storage adapter #3237
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
core/src/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageEnv.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageEnv.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfig.java
Show resolved
Hide resolved
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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 configuration naming inconsistencies and fixes a missing threshold option registration in the S3 adapter. The changes standardize configuration property keys to use underscores instead of hyphens across all object storage adapters (S3, Blob Storage, and Cloud Storage).
Key Changes:
- Renamed configuration properties from hyphenated format (e.g.,
parallel_upload_block_size_in_bytes) to simplified underscore format (e.g.,multipart_upload_part_size_bytesfor S3,parallel_upload_block_size_bytesfor Blob Storage) - Fixed S3Wrapper to properly register the multipart upload threshold option with the S3 client (lines 53-55 in S3Wrapper.java)
- Updated CloudStorageConfig and BlobStorageConfig to use
STORAGE_NAME_IN_PREFIXconstant for configuration key prefixes, replacing hyphens with underscores
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Config.java | Updated constant names from PARALLEL_UPLOAD_*_IN_BYTES to MULTIPART_UPLOAD_*_BYTES and REQUEST_TIMEOUT_IN_SECONDS to REQUEST_TIMEOUT_SECS; renamed corresponding fields and methods |
| core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java | Added missing threshold configuration registration (lines 53-55); updated method calls to use renamed config getters |
| core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfig.java | Introduced STORAGE_NAME_IN_PREFIX constant using underscores; renamed constants from *_IN_BYTES to *_BYTES and *_IN_SECONDS to *_SECS; updated fields and methods accordingly |
| core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageWrapper.java | Updated field name from requestTimeoutInSeconds to requestTimeoutSecs and updated all references to renamed config methods |
| core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfig.java | Introduced STORAGE_NAME_IN_PREFIX constant; renamed PARALLEL_UPLOAD_BLOCK_SIZE_IN_BYTES to UPLOAD_CHUNK_SIZE_BYTES and renamed field/method accordingly |
| core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java | Renamed field from parallelUploadBlockSizeInBytes to uploadMaxChunkSizeBytes and updated references to renamed config methods |
| core/src/test/java/com/scalar/db/storage/objectstorage/s3/S3ConfigTest.java | Updated test constants and assertions to use new configuration property names |
| core/src/test/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfigTest.java | Updated test constants and assertions to use new configuration property names |
| core/src/test/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfigTest.java | Updated test constants and assertions to use new configuration property names |
| core/src/test/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapperTest.java | Updated mock setup to call renamed getUploadChunkSizeBytes() method |
| core/src/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperLargeObjectWriteIntegrationTest.java | Updated property setters to use new constant names; renamed local variables for clarity |
| core/src/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageEnv.java | Updated property setters to use new constant names across all storage adapters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/test/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfigTest.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfigTest.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!
core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java
Show resolved
Hide resolved
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!
|
@KodaiD Can you check the CI failure? |
Description
This PR fixes the option issue specific to Object Storage adapter.
The current code does not pass the threshold option value properly to the client in S3 adapter. Additionally, option names have a '-' character, which is inconsistent with other options.
Related issues and/or PRs
Changes made
S3Wrapperto register the threshold option value.Checklist
Additional notes (optional)
N/A
Release notes
Fixed option issues in Object Storage adapter.