test(spanner): fix retry unit tests with RESOURCE_EXHAUSTED errors #10117
Conversation
rahul2393
added
the
kokoro:force-run
| @@ -731,7 +731,7 @@ func (s *inMemSpannerServer) BatchCreateSessions(ctx context.Context, req *spann | |||
| s.mu.Lock() | |||
| defer s.mu.Unlock() | |||
| if s.maxSessionsReturnedByServerInTotal > int32(0) && int32(len(s.sessions)) >= s.maxSessionsReturnedByServerInTotal { | |||
| return nil, gstatus.Error(codes.ResourceExhausted, "No more sessions available") | |||
| return nil, gstatus.Error(codes.OutOfRange, "No more sessions available") | |||
There was a problem hiding this comment.
Why do we need to change this to a different error code? I didn't understand this change.
There was a problem hiding this comment.
This was the mock response, it has nothing to do with actual cloud spanner backend response code, but unfortunately since ResourceExhausted is treated as retryable now we are changing to some other error code to make it fail and not retry the error which was assumption when writing the test case/this mock code before.
There was a problem hiding this comment.
Got it. Can we document this assumption as a comment on top of the mock response and the test? This way developers can understand the reason behind sending this irrelevant error code?
Also how about marking it as INTERNAL error? OutOfRange error doesn't seem to be relevant to the problem? I will leave it up to you since this mock error code is not serving any purpose.
There was a problem hiding this comment.
Actually OutOfRange suits more here https://developers.google.com/actions-center/reference/grpc-api/status_codes, since in mock code we want to make sure session count never exceeds maxSessionsReturnedByServerInTotal, while Internal Errors should be returned when
Internal errors. This means that some invariants expected by the underlying system have been broken. This error code is reserved for serious errors.
kokoro-team
removed
the
kokoro:force-run
Fixes: #10070, #10069