feat(storage): add retry config to the Client and HmacKey operations (g…

…oogleapis#5193)

storage/bucket.go

@@ -1435,9 +1435,9 @@ func (b *BucketHandle) Objects(ctx context.Context, q *Query) *ObjectIterator {
 // on the new handle will use the customized retry configuration.
 // Retry options set on a object handle will take precedence over options set on
 // the bucket handle.
-// These retry options will merge with the client's retryer (if set) for the
-// returned handle. Options passed into this method will take precedence over
-// options on the client's retryers. Note that you must explicitly pass in each
+// These retry options will merge with the client's retry configuration (if set)
+// for the returned handle. Options passed into this method will take precedence
+// over retry options on the client. Note that you must explicitly pass in each
 // option you want to override.
 func (b *BucketHandle) Retryer(opts ...RetryOption) *BucketHandle {
 	b2 := *b

storage/emulator_test.sh

@@ -58,7 +58,6 @@ trap cleanup EXIT
 # TODO: move to passing once fixed
 FAILING=(   "buckets.setIamPolicy"
             "objects.insert"
-            "hmacKey.update"
         )
 # TODO: remove regex once all tests are passing
 # Unfortunately, there is no simple way to skip specific tests (see https://github.com/golang/go/issues/41583)
@@ -85,6 +84,7 @@ PASSING=(   "buckets.list"
             "hmacKey.list"
             "hmacKey.create"
             "hmacKey.delete"
+            "hmacKey.update"
             "notifications.list"
             "notifications.create"
             "notifications.get"

storage/hmac.go

@@ -216,12 +216,12 @@ func (c *Client) CreateHMACKey(ctx context.Context, projectID, serviceAccountEma
 	setClientHeader(call.Header())
 
 	var hkPb *raw.HmacKey
-	var err error
-	err = run(ctx, func() error {
-		hkPb, err = call.Context(ctx).Do()
+
+	if err := run(ctx, func() error {
+		h, err := call.Context(ctx).Do()
+		hkPb = h
 		return err
-	}, c.retry, false)
-	if err != nil {
+	}, c.retry, false); err != nil {
 		return nil, err
 	}
 
@@ -263,10 +263,11 @@ func (h *HMACKeyHandle) Update(ctx context.Context, au HMACKeyAttrsToUpdate, opt
 
 	var metadata *raw.HmacKeyMetadata
 	var err error
+	isIdempotent := len(au.Etag) > 0
 	err = run(ctx, func() error {
 		metadata, err = call.Context(ctx).Do()
 		return err
-	}, h.retry, true)
+	}, h.retry, isIdempotent)
 
 	if err != nil {
 		return nil, err

storage/retry_conformance_test.go

@@ -252,12 +252,14 @@ var methods = map[string][]retryFunc{
 	"storage.hmacKey.update": {
 		func(ctx context.Context, c *Client, fs *resources, preconditions bool) error {
 			key := c.HMACKeyHandle(projectID, fs.hmacKey.AccessID)
+			uattrs := HMACKeyAttrsToUpdate{State: "INACTIVE"}
 
-			_, err := key.Update(ctx, HMACKeyAttrsToUpdate{State: "INACTIVE"})
-			if err != nil {
-				return err
+			if preconditions {
+				uattrs.Etag = fs.hmacKey.Etag
 			}
-			return fmt.Errorf("Etag preconditions not supported")
+
+			_, err := key.Update(ctx, uattrs)
+			return err
 		},
 	},
 	"storage.objects.compose": {

storage/storage.go

@@ -1793,8 +1793,8 @@ func setConditionField(call reflect.Value, name string, value interface{}) bool
 // on the new handle will use the customized retry configuration.
 // These retry options will merge with the bucket's retryer (if set) for the
 // returned handle. Options passed into this method will take precedence over
-// options on the bucket's and client's retryers. Note that you must explicitly
-// pass in each option you want to override.
+// retry options on the bucket and client. Note that you must explicitly pass in
+// each option you want to override.
 func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle {
 	o2 := *o
 	var retry *retryConfig
@@ -1812,13 +1812,14 @@ func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle {
 	return &o2
 }
 
-// Retryer returns an Client that is configured with custom retry behavior as
-// specified by the options that are passed to it. All operations on the new
-// handle will use the customized retry configuration.
+// SetRetry configures the client with custom retry behavior as specified by the
+// options that are passed to it. All operations using this client will use the
+// customized retry configuration.
+// This should be called once before using the client for network operations, as
+// there could be indeterminate behaviour with operations in progress.
 // Retry options set on a bucket or object handle will take precedence over
 // these options.
-func (c *Client) Retryer(opts ...RetryOption) *Client {
-	c2 := *c
+func (c *Client) SetRetry(opts ...RetryOption) {
 	var retry *retryConfig
 	if c.retry != nil {
 		// merge the options with the existing retry
@@ -1829,8 +1830,7 @@ func (c *Client) Retryer(opts ...RetryOption) *Client {
 	for _, opt := range opts {
 		opt.apply(retry)
 	}
-	c2.retry = retry
-	return &c2
+	c.retry = retry
 }
 
 // RetryOption allows users to configure non-default retry behavior for API

storage/storage_test.go

@@ -875,32 +875,29 @@ func TestObjectRetryer(t *testing.T) {
 	}
 }
 
-// Test that Client.Retryer correctly configures the retry configuration
+// Test that Client.SetRetry correctly configures the retry configuration
 // on the Client.
-func TestClientRetryer(t *testing.T) {
+func TestClientSetRetry(t *testing.T) {
 	testCases := []struct {
-		name string
-		call func(c *Client) *Client
-		want *retryConfig
+		name          string
+		clientOptions []RetryOption
+		want          *retryConfig
 	}{
 		{
-			name: "all defaults",
-			call: func(c *Client) *Client {
-				return c.Retryer()
-			},
-			want: &retryConfig{},
+			name:          "all defaults",
+			clientOptions: []RetryOption{},
+			want:          &retryConfig{},
 		},
 		{
 			name: "set all options",
-			call: func(c *Client) *Client {
-				return c.Retryer(
-					WithBackoff(gax.Backoff{
-						Initial:    2 * time.Second,
-						Max:        30 * time.Second,
-						Multiplier: 3,
-					}),
-					WithPolicy(RetryAlways),
-					WithErrorFunc(func(err error) bool { return false }))
+			clientOptions: []RetryOption{
+				WithBackoff(gax.Backoff{
+					Initial:    2 * time.Second,
+					Max:        30 * time.Second,
+					Multiplier: 3,
+				}),
+				WithPolicy(RetryAlways),
+				WithErrorFunc(func(err error) bool { return false }),
 			},
 			want: &retryConfig{
 				backoff: &gax.Backoff{
@@ -914,11 +911,10 @@ func TestClientRetryer(t *testing.T) {
 		},
 		{
 			name: "set some backoff options",
-			call: func(c *Client) *Client {
-				return c.Retryer(
-					WithBackoff(gax.Backoff{
-						Multiplier: 3,
-					}))
+			clientOptions: []RetryOption{
+				WithBackoff(gax.Backoff{
+					Multiplier: 3,
+				}),
 			},
 			want: &retryConfig{
 				backoff: &gax.Backoff{
@@ -927,18 +923,17 @@ func TestClientRetryer(t *testing.T) {
 		},
 		{
 			name: "set policy only",
-			call: func(c *Client) *Client {
-				return c.Retryer(WithPolicy(RetryNever))
+			clientOptions: []RetryOption{
+				WithPolicy(RetryNever),
 			},
 			want: &retryConfig{
 				policy: RetryNever,
 			},
 		},
 		{
 			name: "set ErrorFunc only",
-			call: func(c *Client) *Client {
-				return c.Retryer(
-					WithErrorFunc(func(err error) bool { return false }))
+			clientOptions: []RetryOption{
+				WithErrorFunc(func(err error) bool { return false }),
 			},
 			want: &retryConfig{
 				shouldRetry: func(err error) bool { return false },
@@ -947,7 +942,13 @@ func TestClientRetryer(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(s *testing.T) {
-			c := tc.call(&Client{})
+			c, err := NewClient(context.Background())
+			if err != nil {
+				t.Fatalf("NewClient: %v", err)
+			}
+			defer c.Close()
+			c.SetRetry(tc.clientOptions...)
+
 			if diff := cmp.Diff(
 				c.retry,
 				tc.want,
@@ -1156,7 +1157,7 @@ func TestRetryer(t *testing.T) {
 			}
 			defer c.Close()
 			if len(tc.clientOptions) > 0 {
-				c = c.Retryer(tc.clientOptions...)
+				c.SetRetry(tc.clientOptions...)
 			}
 			b := c.Bucket("buck")
 			if len(tc.bucketOptions) > 0 {