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.

Sign up for GitHub

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

Jump to bottom

feat(storage): add retry config to ACL handle methods #5185

Merged
merged 7 commits into from Dec 9, 2021
Merged

feat(storage): add retry config to ACL handle methods #5185

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
965a217
feat(storage): add retry config to ACL handle methods
BrennaEpp Dec 3, 2021
69a6765
Merge branch 'main' into acl-retry
BrennaEpp Dec 3, 2021
66efa2f
Merge branch 'main' into acl-retry
BrennaEpp Dec 3, 2021
5813a7f
Merge branch 'main' into acl-retry
BrennaEpp Dec 6, 2021
f64dfc8
fill acl methods
BrennaEpp Dec 8, 2021
9628207
Merge branch 'main' into acl-retry
BrennaEpp Dec 8, 2021
5ef29a8
Merge branch 'main' into acl-retry
BrennaEpp Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 25 additions & 13 deletions storage/acl.go
View file
Open in desktop
Expand Up @@ -73,6 +73,7 @@ type ACLHandle struct {
object string
isDefault bool
userProject string // for requester-pays buckets
retry *retryConfig
}

// Delete permanently deletes the ACL entry for the given entity.
Expand Down Expand Up @@ -120,12 +121,12 @@ func (a *ACLHandle) List(ctx context.Context) (rules []ACLRule, err error) {
func (a *ACLHandle) bucketDefaultList(ctx context.Context) ([]ACLRule, error) {
var acls *raw.ObjectAccessControls
var err error
err = runWithRetry(ctx, func() error {
err = run(ctx, func() error {
req := a.c.raw.DefaultObjectAccessControls.List(a.bucket)
a.configureCall(ctx, req)
acls, err = req.Do()
return err
})
}, a.retry, true)
if err != nil {
return nil, err
}
Expand All @@ -135,18 +136,21 @@ func (a *ACLHandle) bucketDefaultList(ctx context.Context) ([]ACLRule, error) {
func (a *ACLHandle) bucketDefaultDelete(ctx context.Context, entity ACLEntity) error {
req := a.c.raw.DefaultObjectAccessControls.Delete(a.bucket, string(entity))
a.configureCall(ctx, req)
return req.Do()

return run(ctx, func() error {
return req.Do()
}, a.retry, false)
}

func (a *ACLHandle) bucketList(ctx context.Context) ([]ACLRule, error) {
var acls *raw.BucketAccessControls
var err error
err = runWithRetry(ctx, func() error {
err = run(ctx, func() error {
req := a.c.raw.BucketAccessControls.List(a.bucket)
a.configureCall(ctx, req)
acls, err = req.Do()
return err
})
}, a.retry, true)
if err != nil {
return nil, err
}
Expand All @@ -161,25 +165,29 @@ func (a *ACLHandle) bucketSet(ctx context.Context, entity ACLEntity, role ACLRol
}
req := a.c.raw.BucketAccessControls.Update(a.bucket, string(entity), acl)
a.configureCall(ctx, req)
_, err := req.Do()
return err
return run(ctx, func() error {
_, err := req.Do()
return err
}, a.retry, false)
}

func (a *ACLHandle) bucketDelete(ctx context.Context, entity ACLEntity) error {
req := a.c.raw.BucketAccessControls.Delete(a.bucket, string(entity))
a.configureCall(ctx, req)
return req.Do()
return run(ctx, func() error {
return req.Do()
}, a.retry, false)
}

func (a *ACLHandle) objectList(ctx context.Context) ([]ACLRule, error) {
var acls *raw.ObjectAccessControls
var err error
err = runWithRetry(ctx, func() error {
err = run(ctx, func() error {
req := a.c.raw.ObjectAccessControls.List(a.bucket, a.object)
a.configureCall(ctx, req)
acls, err = req.Do()
return err
})
}, a.retry, true)
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
Expand All @@ -204,14 +212,18 @@ func (a *ACLHandle) objectSet(ctx context.Context, entity ACLEntity, role ACLRol
req = a.c.raw.ObjectAccessControls.Update(a.bucket, a.object, string(entity), acl)
}
a.configureCall(ctx, req)
_, err := req.Do()
return err
return run(ctx, func() error {
_, err := req.Do()
return err
}, a.retry, false)
}

func (a *ACLHandle) objectDelete(ctx context.Context, entity ACLEntity) error {
req := a.c.raw.ObjectAccessControls.Delete(a.bucket, a.object, string(entity))
a.configureCall(ctx, req)
return req.Do()
return run(ctx, func() error {
return req.Do()
}, a.retry, false)
}

func (a *ACLHandle) configureCall(ctx context.Context, call interface{ Header() http.Header }) {
Expand Down
6 changes: 5 additions & 1 deletion storage/bucket.go
View file
Open in desktop
Expand Up @@ -146,6 +146,7 @@ func (b *BucketHandle) DefaultObjectACL() *ACLHandle {
// for valid object names can be found at:
// https://cloud.google.com/storage/docs/naming-objects
func (b *BucketHandle) Object(name string) *ObjectHandle {
retry := b.retry.clone()
return &ObjectHandle{
c: b.c,
bucket: b.name,
Expand All @@ -155,10 +156,11 @@ func (b *BucketHandle) Object(name string) *ObjectHandle {
bucket: b.name,
object: name,
userProject: b.userProject,
retry: retry,
},
gen: -1,
userProject: b.userProject,
retry: b.retry.clone(),
retry: retry,
}
}

Expand Down Expand Up @@ -1436,6 +1438,8 @@ func (b *BucketHandle) Retryer(opts ...RetryOption) *BucketHandle {
opt.apply(retry)
}
b2.retry = retry
b2.acl.retry = retry
b2.defaultObjectACL.retry = retry
return &b2
}

Expand Down
1 change: 1 addition & 0 deletions storage/storage.go
View file
Open in desktop
Expand Up @@ -1806,6 +1806,7 @@ func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle {
opt.apply(retry)
}
o2.retry = retry
o2.acl.retry = retry
return &o2
}

Expand Down
54 changes: 42 additions & 12 deletions storage/storage_test.go
View file
Open in desktop
Expand Up @@ -876,7 +876,7 @@ func TestObjectRetryer(t *testing.T) {
}

// Test the interactions between ObjectHandle and BucketHandle Retryers and that
// they correctly configure the retry configuration
// they correctly configure the retry configuration for objects and ACLs
func TestRetryer(t *testing.T) {
testCases := []struct {
name string
Expand Down Expand Up @@ -1007,17 +1007,47 @@ func TestRetryer(t *testing.T) {
o = o.Retryer(tc.objectOptions...)
}

if diff := cmp.Diff(
o.retry,
tc.want,
cmp.AllowUnexported(retryConfig{}, gax.Backoff{}),
// ErrorFunc cannot be compared directly, but we check if both are
// either nil or non-nil.
cmp.Comparer(func(a, b func(err error) bool) bool {
return (a == nil && b == nil) || (a != nil && b != nil)
}),
); diff != "" {
s.Fatalf("retry not configured correctly: %v", diff)
configHandleCases := []struct {
r *retryConfig
name string
want *retryConfig
}{
{
name: "object.retry",
r: o.retry,
want: tc.want,
},
{
name: "object.ACL()",
r: o.ACL().retry,
want: tc.want,
},
{
name: "bucket.ACL()",
r: b.ACL().retry,
want: b.retry,
},
{
name: "bucket.DefaultObjectACL()",
r: b.DefaultObjectACL().retry,
want: b.retry,
},
}
for _, ac := range configHandleCases {
s.Run(ac.name, func(ss *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realize these could be nested... nice!

if diff := cmp.Diff(
ac.want,
ac.r,
cmp.AllowUnexported(retryConfig{}, gax.Backoff{}),
// ErrorFunc cannot be compared directly, but we check if both are
// either nil or non-nil.
cmp.Comparer(func(a, b func(err error) bool) bool {
return (a == nil && b == nil) || (a != nil && b != nil)
}),
); diff != "" {
ss.Fatalf("retry not configured correctly: %v", diff)
}
})
}
})
}
Expand Down