-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(storage): retry gRPC DEADLINE_EXCEEDED errors #10635
Conversation
GCS is in some cases returning these for internal timeouts, so they need to be retried. I added an extra context error check to our retry logic to ensure that they can always be distinguished from user-set deadlines as well as some tests. See internal bug b/
// status when it is returned by the server. | ||
func TestTimeoutErrorEmulated(t *testing.T) { | ||
transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused by this test, is this to cause DeadlineExceeded client-side while the other is meant for server side (storage-testbench) to cause DeadlineExceeded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. We want to make sure that client-set deadlines are not retried (to avoid issue mentioned in https://google.aip.dev/194#non-retryable-codes ) but that a DEADLINE_EXCEEDED returned by the server is retried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't speak golang well enough to understand the tests, but the functional code LGTM
GCS is in some cases returning these for internal timeouts, so they need to be retried. I added an extra context error check to our retry logic to ensure that they can always be distinguished from user-set deadlines as well as some tests.
See internal bug b/333307965
Requires googleapis/storage-testbench#663
Fixes #9827