Skip to content
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

chore(storage): RewriteObject implementation #6313

Merged
merged 11 commits into from
Jul 20, 2022

Conversation

noahdietz
Copy link
Contributor

Implements the RewriteObject transport-agnostic interface methods for both HTTP and gRPC. This API is used to power the Object Copy functionality of the client.

@noahdietz noahdietz requested review from a team as code owners July 7, 2022 20:38
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the Cloud Storage API. labels Jul 7, 2022
@noahdietz noahdietz requested review from tritone and cojenco July 7, 2022 20:42
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, generally looks great! A few minor things.

@noahdietz noahdietz requested a review from tritone July 20, 2022 00:07
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more note from the refactoring; otherwise LGTM

@@ -783,7 +783,7 @@ func (c *grpcStorageClient) RewriteObject(ctx context.Context, req *rewriteObjec
if s.userProject != "" {
ctx = setUserProjectMetadata(ctx, s.userProject)
}
if err := applyCondsProto("Copy destination", req.gen, req.conds, call); err != nil {
if err := applyCondsProto("Copy destination", req.srcObject.gen, req.dstObject.conds, call); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not correct -- you should just pass -1 for generation to indicate the latest. Passing a specific generation for a destination object is not possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for http.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I thought that was weird when I wrote it...switched it to defaultGen. Is that right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup looks good now! Thanks!

@noahdietz noahdietz requested a review from tritone July 20, 2022 16:36
@noahdietz noahdietz merged commit e8d24c1 into googleapis:main Jul 20, 2022
@noahdietz noahdietz deleted the rewrite-object branch July 20, 2022 17:25
noahdietz added a commit that referenced this pull request Jul 21, 2022
* chore: release main (#6351)

* test(profiler): compile busybench before running backoff test (#6375)

* chore(bigquery/storage/managedwriter): augment test logging (#6373)

* chore(storage): RewriteObject implementation (#6313)

* chore(storage): RewriteObject implementation

* address feedback

* refactor source/destination object types

* address feedback

* address feedback

* fix test

* chore(main): release storage 1.24.0 (#6336)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Chris Cotter <[email protected]>

* chore(internal/gapicgen): update microgen v0.31.2 (#6383)

Only includes fixes to regapic generation.

* test(bigquery/storage/managedwriter): relax error checking (#6385)

When a user issues a large request, the response from the
backend is a bare "InvalidArgument".  This PR removes additional
validation on information that is only attached when interrogating
the backend from a known client; it's stripped in the normal case.

Internal issue 239740070 was created to address the unactionable
nature of the response.

Fixes: #6361

* feat(firestore): adds Bulkwriter support to Firestore client (#5946)

* feat: adds Bulkwriter support to Firestore client

* test(storage): unflake TestIntegration_ACL (#6392)

Few minor changes to make sure potentially flaky and/or
eventually consistent operations for ACLs are retried appropriately.

Fixes #6379

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Amarin (Um) Phaosawasdi <[email protected]>
Co-authored-by: shollyman <[email protected]>
Co-authored-by: Noah Dietz <[email protected]>
Co-authored-by: Chris Cotter <[email protected]>
Co-authored-by: Eric Schmidt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants