Retry GoogleDataTransport Connection Errors #1705
Conversation
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (94d853c7) is created by Prow via merging commits: 037282f e2af542. |
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (94d853c7) is created by Prow via merging commits: 037282f e2af542. |
|
/test smoke-tests |
|
Links were super helpful. This would result in retrying ? What am i missing |
|
@ashwinraghav The issue is IOException from this error don't bubble up to that So when this error happens that we are testing, we get a valid response here. And then it becomes a fatalError here before, because it was a 400. Not since we return a |
Makes sense. |
|
Can you add a test similar to |
| @@ -268,6 +270,9 @@ private HttpResponse doSend(HttpRequest request) throws IOException { | |||
| // JsonWriter often writes one character at a time. | |||
| dataEncoder.encode( | |||
| request.requestBody, new BufferedWriter(new OutputStreamWriter(outputStream))); | |||
| } catch (ConnectException | UnknownHostException e) { | |||
There was a problem hiding this comment.
Thinking out loud: This seems like a pretty conservative approach, as opposed to treating all IO Exceptions as retry-able.
There was a problem hiding this comment.
Yeah exactly - the reason we did this is there are legit IO Exceptions that would not be retryable, eg. encoding errors of some sort
In cases where there is bad connection, the current behavior deletes the cached reports because they are caught as an
IOException. They appear to be encoding errors, when in reality it's an error opening theOutputStreamthat is the connection.This changes it so that when the
IOExceptionis due to connection problems, we retry the upload instead of deleting it.This is where we transform status numbers into success, fatal, and transient errors: CctTransportBackend.java
This is where we decide to delete fatal errors, and retry transient errors: Uploader.java