-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add support for partial success in ListBuckets for grpc #1
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: list-buckets
Are you sure you want to change the base?
Conversation
| .map( | ||
| name -> { | ||
| String encoded = bucketNameCodec.encode(name); | ||
| BucketInfo.Builder builder = BucketInfo.newBuilder(encoded); |
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.
nit: can chain the setters of this builder
| // New logic for partial success | ||
| try { | ||
| GrpcCallContext merge = Utils.merge(grpcCallContext, Retrying.newCallContext()); | ||
| com.google.storage.v2.ListBucketsResponse response = |
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.
this logic seems to be repeated multiple times including implementation of ListBucketsWithPartialSuccessPage, can we can have a helper method for this ?
| page.iterateAll().forEach(allBuckets::add); | ||
|
|
||
| // Verify we found all expected buckets | ||
| assertThat( |
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.
we can use other syntaxes of assertThat made specifically for assertion on collection.
Ex: assertThat(actualList).containsExactlyInAnyOrder(elem1, elem2)
| throw StorageException.coalesce(e); | ||
| } | ||
| } else { | ||
| // New logic for partial success |
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.
remove redundant comment
| throw StorageException.coalesce(e); | ||
| .apply(ListBucketsRequest.newBuilder()); | ||
|
|
||
| final ListBucketsRequest request = builder.build(); |
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.
any reason for breaking this to new line, rather than keeping it above as ListBucketRequest
PR adds support for partial list buckets for GRPC.