firestore: BatchGetDocuments does not use Default Call Options

[ABevier]

Client

firestore

Environment

golang:1.18-buster on Cloud Run

Go Environment

Go 1.18

Code

The default call options for BatchGetDocuments are setup on line 127 of firestore_client.go:

		BatchGetDocuments: []gax.CallOption{
			gax.WithRetry(func() gax.Retryer {
				return gax.OnCodes([]codes.Code{
					codes.ResourceExhausted,
					codes.Unavailable,
					codes.Internal,
					codes.DeadlineExceeded,
				}, gax.Backoff{
					Initial:    100 * time.Millisecond,
					Max:        60000 * time.Millisecond,
					Multiplier: 1.30,
				})
			}),
		},

However the options are not used in BatchGetDocuments on line 611 of firestore_client.go:

func (c *gRPCClient) BatchGetDocuments(ctx context.Context, req *firestorepb.BatchGetDocumentsRequest, opts ...gax.CallOption) (firestorepb.Firestore_BatchGetDocumentsClient, error) {
	md := metadata.Pairs("x-goog-request-params", fmt.Sprintf("%s=%v", "database", url.QueryEscape(req.GetDatabase())))

	ctx = insertMetadata(ctx, c.xGoogMetadata, md)
	var resp firestorepb.Firestore_BatchGetDocumentsClient
	err := gax.Invoke(ctx, func(ctx context.Context, settings gax.CallSettings) error {
		var err error
		resp, err = c.client.BatchGetDocuments(ctx, req, settings.GRPC...)
		return err
	}, opts...)
	if err != nil {
		return nil, err
	}
	return resp, nil
}

Expected behavior

The BatchGetDocuments function should use the default call options of the firestore client. I would expect to find the following line of code in BatchGetDocuments:

	opts = append((*c.CallOptions).BatchGetDocuments[0:len((*c.CallOptions).BatchGetDocuments):len((*c.CallOptions).BatchGetDocuments)], opts...)

Actual behavior

No call options are passed to BatchGetDocuments which prevent automatic retries when Unavailable errors are encountered.

[telpirion]

See here:
https://github.com/googleapis/google-cloud-go/blob/main/firestore/apiv1/firestore_client.go#L620

[ABevier]

Hi @telpirion,

I think there is a misunderstanding here. While it is true that the opts are passed to the call on line 620, the default call options are not appended and opts is ultimately empty when invoked by the firestore sdk. We discovered this issue when debugging why the firestore client does not retry unavailable status errors for BatchGetDocuments despite being configured to do so.

The line

	opts = append((*c.CallOptions).BatchGetDocuments[0:len((*c.CallOptions).BatchGetDocuments):len((*c.CallOptions).BatchGetDocuments)], opts...)

will fix this issue.

Please also note that an equivalent line is present for the other api calls, and that this line is also present for BatchGetDocuments in the generated code for apiv1beta.

Without a fix for this issue we have to implement our own retry policy for calls that result in BatchGetDocuments

[enocom]

@ABevier Sorry for the slow response here.

This is indeed a mistake and can be fixed in the autogenerated code. The PR linked above will restore the call options to the streaming calls.

[ABevier]

@enocom - Thanks so much for looking deeper into this!

[enocom]

We're currently in a release freeze that won't be lifted until January. When that happens, we'll merge the generator PR above, and then regenerate the firestore client which will finally close this issue. I'll update here when that work is done.

[enocom]

This will be fixed in #7232.