-
Notifications
You must be signed in to change notification settings - Fork 4.5k
GCS client library migration in Java SDK - part 2 #37502
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
d5d0b08 to
15d8bcc
Compare
b3ad450 to
b4355c6
Compare
|
R: @Abacn |
|
/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 is a significant step in migrating the GCS client library to the newer version. The approach of using a feature flag and a facade (GcsUtil) to delegate to either the V1 or V2 implementation is sound and allows for a safe, gradual migration. The addition of parameterized integration tests to ensure behavioral consistency between the two versions is excellent.
I have identified a critical bug in the batch getBlobs implementation that could lead to incorrect error messages. Additionally, I've found an opportunity to improve the efficiency and atomicity of the removeBucket method. There's also a minor, but repeated, typo in error messages across several new methods. Addressing these points will further improve the quality of this contribution.
...gle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV2.java
Show resolved
Hide resolved
...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java
Outdated
Show resolved
Hide resolved
...gle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV2.java
Show resolved
Hide resolved
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java
Outdated
Show resolved
Hide resolved
...gle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV2.java
Outdated
Show resolved
Hide resolved
...gle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtilV2.java
Show resolved
Hide resolved
...d-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/util/gcsfs/GcsPathTest.java
Show resolved
Hide resolved
Abacn
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!
This is the second part of GCS client library migration for most of the blob/bucket metadata manipulation methods. A parameterized test suite is also added to make sure the behavior of GcsUtil V2 is consistent with V1 as much as it could be.
Objects/Blobs:
fileSize()getObject()(deprecated and the new api isgetBlob())getObjects()(deprecated and the new api isgetBlobs())listObjects()(deprecated and the new api islistBlobs())expand()Buckets:
getBucket()(deprecated and the new api isgetBucketWithOptions()bucketAccessible()verifyBucketAccessible()bucketOwner()createBucket()removeBucket()Notice that: