Skip to content

Commit

Permalink
fix(storage): disable gax retries for gRPC (#9747)
Browse files Browse the repository at this point in the history
All gRPC ops are retried as needed at the handwritten layer. Disable gax retries.
  • Loading branch information
tritone authored Apr 11, 2024
1 parent 6a7cd4f commit bbfc0ac
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 1 deletion.
46 changes: 46 additions & 0 deletions storage/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package storage

import (
"context"
"errors"
"fmt"
"log"
"net/url"
"os"
"strconv"
"strings"
Expand All @@ -26,7 +28,10 @@ import (

"cloud.google.com/go/iam/apiv1/iampb"
"github.com/google/go-cmp/cmp"
"github.com/googleapis/gax-go/v2/apierror"
"github.com/googleapis/gax-go/v2/callctx"
"google.golang.org/api/iterator"
"google.golang.org/grpc/codes"
)

var emulatorClients map[string]storageClient
Expand Down Expand Up @@ -1342,6 +1347,47 @@ func TestObjectConditionsEmulated(t *testing.T) {
})
}

// Test that RetryNever prevents any retries from happening in both transports.
func TestRetryNeverEmulated(t *testing.T) {
transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
ctx := context.Background()

attrs, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil)
if err != nil {
t.Fatalf("creating bucket: %v", err)
}

// Need the HTTP hostname to set up a retry test, as well as knowledge of
// underlying transport to specify instructions.
host := os.Getenv("STORAGE_EMULATOR_HOST")
endpoint, err := url.Parse(host)
if err != nil {
t.Fatalf("parsing endpoint: %v", err)
}
var transport string
if _, ok := client.(*httpStorageClient); ok {
transport = "http"
} else {
transport = "grpc"
}

et := emulatorTest{T: t, name: "testRetryNever", resources: resources{},
host: endpoint}
et.create(map[string][]string{"storage.buckets.get": {"return-503"}}, transport)
ctx = callctx.SetHeaders(ctx, "x-retry-test-id", et.id)
_, err = client.GetBucket(ctx, attrs.Name, nil, withRetryConfig(&retryConfig{policy: RetryNever}))

var ae *apierror.APIError
if errors.As(err, &ae) {
// We espect a 503/UNAVAILABLE error. For anything else including a nil
// error, the test should fail.
if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 {
t.Errorf("GetBucket: got unexpected error %v; want 503", err)
}
}
})
}

// createObject creates an object in the emulator and returns its name, generation, and
// metageneration.
func createObject(ctx context.Context, bucket string) (string, int64, int64, error) {
Expand Down
2 changes: 2 additions & 0 deletions storage/grpc_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ type grpcStorageClient struct {
func newGRPCStorageClient(ctx context.Context, opts ...storageOption) (storageClient, error) {
s := initSettings(opts...)
s.clientOption = append(defaultGRPCOptions(), s.clientOption...)
// Disable all gax-level retries in favor of retry logic in the veneer client.
s.gax = append(s.gax, gax.WithRetry(nil))

config := newStorageConfig(s.clientOption...)
if config.readAPIWasSet {
Expand Down
5 changes: 4 additions & 1 deletion storage/retry_conformance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,9 @@ var methods = map[string][]retryFunc{
}

func TestRetryConformance(t *testing.T) {
// This endpoint is used only to call the testbench retry test API, which is HTTP
// based. The endpoint called by the client library is determined inside of the
// client constructor and will differ depending on the transport.
host := os.Getenv("STORAGE_EMULATOR_HOST")
if host == "" {
t.Skip("This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run.")
Expand Down Expand Up @@ -759,7 +762,7 @@ func (et *emulatorTest) create(instructions map[string][]string, transport strin

et.host.Path = "retry_test"
resp, err := c.Post(et.host.String(), "application/json", buf)
if resp.StatusCode == 501 {
if resp != nil && resp.StatusCode == 501 {
et.T.Skip("This retry test case is not yet supported in the testbench.")
}
if err != nil || resp.StatusCode != 200 {
Expand Down

0 comments on commit bbfc0ac

Please sign in to comment.