feat(spanner): support of client-level custom retry settings

[hengfengli]

Changes included are:

• Add a field CallOptions in ClientConfig.
• Add a field callOptions in sessionClient.
• Add a method mergeCallOptions to merge two CallOptions.
• Update nextClient in sessionclient.go so that if a custom CallOptions is provided, merge it with the default one.

Fixes #2594

[olavloite]

I really like how clean and easy this seems to make passing in custom retry settings, but I'm a little bit confused about how the merging of the custom settings with the default settings works. Would you mind explaining that a little?

[olavloite]

I'm not very familiar with the exact working of these options in Go. When exactly would the merged.CreateSession[1] setting be used instead of the merged.CreateSession[0] setting?

[hengfengli]

google-cloud-go/spanner/apiv1/spanner_client.go

Lines 308 to 311 in 06b6069

	func (c *Client) CreateSession(ctx context.Context, req *spannerpb.CreateSessionRequest, opts ...gax.CallOption) (*spannerpb.Session, error) {
	md := metadata.Pairs("x-goog-request-params", fmt.Sprintf("%s=%v", "database", url.QueryEscape(req.GetDatabase())))
	ctx = insertMetadata(ctx, c.xGoogMetadata, md)
	opts = append(c.CallOptions.CreateSession[0:len(c.CallOptions.CreateSession):len(c.CallOptions.CreateSession)], opts...)

In the generated method, you can see that opts = [c.CallOptions.CreateSession, opts...]. So the passed-in opts have higher order of precedence.

Therefore, merged.CreateSession[1] will override merged.CreateSession[0].

My guess is that it will iterate all gax.CallOption in merged.CreateSession and call Resolve with gax.CallSettings. Only the effect of the last one remains. Then, it uses gax.CallSettings for calling gRPC.

[olavloite]

Is this the setting that will actually be used in this case? Is it in that case necessary to keep the other settings still in c.CallOptions.CreateSession[0]?

[hengfengli]

As I described in the previous comment, c.CallOptions.CreateSession[1] has higher order of precedence. Yes, it is not necessary to keep c.CallOptions.CreateSession[0]. But I have two reasons to do it in this way:

• Follow the same pattern in spanner_client.go: opts = append(c.CallOptions.CreateSession[0:len(c.CallOptions.CreateSession):len(c.CallOptions.CreateSession)], opts...)
• Simplify the merging logic: we don't need to say if b is not empty, use b, else if a is not empty, use a, and if both empty, leave it empty. We can just merge a and b in the right order.

[olavloite]

Sounds reasonable to me. I do think we should consider adding some kind of end-to-end test for this, as we are relying on what is effectively an undocumented feature of the generated client that might silently change in the future. That end-to-end test could probably be created using the mock server, and does not need to be a full integration test.

[hengfengli]

The default CallOptions are set in:

google-cloud-go/spanner/apiv1/spanner_client.go

Lines 256 to 261 in 06b6069

	c := &Client{
	connPool: connPool,
	CallOptions: defaultCallOptions(),
	client: spannerpb.NewSpannerClient(connPool),
	}

We override the default CallOptions in:

google-cloud-go/spanner/sessionclient.go

Lines 279 to 281 in c455fe2

	if sc.callOptions != nil {
	client.CallOptions = mergeCallOptions(client.CallOptions, sc.callOptions)
	}

client.CallOptions is the default CallOptions and sc.callOptions is the custom CallOptions.

Assume client.CallOptions is:

client.CallOptions = {
   CreateSession: [opt1],
   BatchCreateSessions: [op2],
   GetSession: [],
}

and sc.callOptions is:

sc.callOptions = {
   CreateSession: [opt3],
   BatchCreateSessions: [],
   GetSession: [op5],
}

Then, the merged one will become:

{
   CreateSession: [opt1, opt3],
   BatchCreateSessions: [opt2],
   GetSession: [op5],
}

The final effect would be CreateSession: opt3, BatchCreateSessions: opt2, GetSession: opt5. The last option in each array takes effect.

[olavloite]

Thank you for the elaborate explanation, that makes it a lot clearer for me.

This seems good to me, but as noted in my comment above, I do think we should consider adding an end-to-end test as well, as it is relying on what seems to be an undocumented feature of the generated client (the undocumented feature being that the generated client will use the last element of the array as its actual call options).

[hengfengli]

I dig a bit more into the code and found:

https://github.com/googleapis/gax-go/blob/be11bb253a768098254dc71e95d1a81ced778de3/v2/invoke.go#L41-L49

It actually does what I think. It iterates all CallOptions and calling Resolve() with CallSettings.

Resolve() just overrides the Retry field of CallSettings:

https://github.com/googleapis/gax-go/blob/be11bb253a768098254dc71e95d1a81ced778de3/v2/call_option.go#L57-L59

Therefore, only the last one will be remained in the Retry field.

But I am happy to add an end-to-end test for this.

[hengfengli]

@olavloite , I have added an end-to-end test. Please take a look.

I learned an interesting fact that the gax retrying does not actually work for ExecuteStreamingSql(). When we call ExecuteStreamingSql(), we actually get a receiver, which does not receive any result yet. It only gets results when we call iter.Next() where the gax retrying will not work.

Basically, what I learned is:

• Create an iterator: create a spannerExecuteStreamingSqlClient (gax retrying) - sending messages to the stream.
• Iterate an iterator: make a ExecuteStreamingSql request (no gax retrying) - receiving results from the stream.

Originally, I tried to set a custom retry setting for ExecuteStreamingSql(), but it doesn't work. So I changed it to use ExecuteSql().

[olavloite]

@olavloite , I have added an end-to-end test. Please take a look.

I learned an interesting fact that the gax retrying does not actually work for ExecuteStreamingSql(). When we call ExecuteStreamingSql(), we actually get a receiver, which does not receive any result yet. It only gets results when we call iter.Next() where the gax retrying will not work.

Basically, what I learned is:

• Create an iterator: create a spannerExecuteStreamingSqlClient (gax retrying) - sending messages to the stream.
• Iterate an iterator: make a ExecuteStreamingSql request (no gax retrying) - receiving results from the stream.

Originally, I tried to set a custom retry setting for ExecuteStreamingSql(), but it doesn't work. So I changed it to use ExecuteSql().

@hengfengli Thanks for adding the end to end test. This looks good to me.

The retry logic for ExecuteStreamingSql is currently hand-written, partly also because it should use the last seen resume token that was received from the server. So if we would want to allow users to specify custom retry settings for queries as well, then we would have to make this retryer configurable.