googleapis / java-spanner

Apache License 2.0
55 stars 110 forks source link

RetrySettings for StreamingRead/StreamingSql #2320

Open dominity opened 1 year ago

dominity commented 1 year ago

Thanks for stopping by to let us know something could be better!

Is your feature request related to a problem? Please describe. Recently I was working on fine tunning timeouts/retry settings for Spanner requests for one of applications. I noticed that initRpcTimeout doesn't works on StreamingRead/StreamingSql type of requests. So I started to go through source code and found that there are 3 places/configurations where I can work with timeouts:

First two aren't configurable from RetrySettings level. Could you please makes them part of RetrySettings, so I don't have to interact with GrpcCallContext directly?

Describe the solution you'd like Either to have separate set of properties in RetrySettings like (and recalculate them similarly to initRpcTimeout during retries):

var settings = RetrySettings.newBuilder()
                    .setStreamWaitTimeout(Duration.ofMillis(streamWaitTimeout))
                    .setSingleRequestTimeout(Duration.ofMillis(singleRequestTimeout)) // used as grpc-timeout

or reuse initRpcTimeout as streamWaitTimeout/grpc-timeout:

var settings = RetrySettings.newBuilder()
                    .setInitialRpcTimeout(Duration.ofMillis(initialRpcTimeout)) // used as both streamingWaitTimeout and grpc-timeout

Describe alternatives you've considered Currently I interact with GrpcCallContext directly to set this values:

    public static class SpannerGrpcCallContextConfigurator implements SpannerOptions.CallContextConfigurator {
        public <ReqT, RespT> ApiCallContext configure(ApiCallContext context, ReqT request,
                MethodDescriptor<ReqT, RespT> method) {
            if (method == SpannerGrpc.getStreamingReadMethod()) {
                return GrpcCallContext.createDefault().
                        .withTimeout(/*some timeout*/)
                        .withStreamWaitTimeout(/*some timeout*/);
            }
            return null;
        }
    }
rajatbhatta commented 1 year ago

This is a feature request. Assigning to @arpan14 to take it forward.