RestClient: on retry timeout add root exception

[Timshel]

No description provided.

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[cbuescher]

LGTM, will kick of a CI build before merging this.

[javanna]

can we have a test for this?

[Timshel]

I just realized that a better fix might be to move listener.trackFailure(exception); before the if (timeout <= 0) block this way the Exception would be added to the suppressed list as usual.

[javanna]

hi @Timshel I think I prefer the original fix that you proposed. the timeout calls listener.onDefinitiveFailure straightaway. I think it would be wrong to call trackFailure right before that as well. I think it's good to keep track of the original exception as part of the retry timeout IOException, so that we know what caused the retries. What do you think?

[Timshel]

From what I understand calling first trackFailure will add the root exception to FailureTrackingResponseListener and then the following call to onDefinitiveFailure will have for result to add this root exception in the suppressed list of the IOException.

If we take two different scenario (with multiple available nodes in both case) :

• 1 : the timeout delay is set to 0 there is no retry
• 2 : the timeout is sufficient for one retry which fail

With no fix the result look like :

• 1 : IOException()
• 2 : IOException(suppressed: [Call1Exception])

With the current PR fix the resulting IOException look like :

• 1 : IOException(cause: Call1Exception)
• 2 : IOException(cause: Call2Exception, suppressed: [Call1Exception])

If the trackFailure is always called before checking the time left it will look like :

• 1 : IOException(suppressed: [Call1Exception])
• 2 : IOException(suppressed: [Call2Exception(supressed: Call1Exception)])

At first I thought always having the exceptions in suppressed might be better, but after writing it, I think always having the exception of the last call in cause (easier to access than the suppressed list) might be better.

So in the end I think the current PR is better (Sorry for the complications :).

[javanna]

hey @Timshel thanks for the explanation. I think we are in agreement, but I have another question: when the retry timeout is set to 0, what would be the suppressed exception if there were no previous retries? Seems to me that there would be no cause, but I may be missing something.

I am also considering changing the behaviour here to rely solely on socket and connect timeout from the underlying http client for the first attempt, without having the maxRetryTimeout affect it. retry timeout instead should kick from the second attempt on. What do you think about this?

[Timshel]

When the retry is set to 0, the exception is the initial error. In my exemple Call2Exception is the first retry call.

I'm not sure I understand, if by first attempt you mean first retry then I don't believe it's possible because startTime is the start of the original query.
When timeout > 0 it could be set as the timeout of the retry query.

[Timshel]

Is there anything blocking the merge of this PR ?

[nik9000]

@javanna, I think this one is at least missing a test. Maybe it needs to change in some way based on the conversation you had above? Can you comment?

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[jasontedor]

@elasticmachine test this please

[jasontedor]

LGTM. I don't think adding a test here is necessary, we don't add tests everywhere that we catch and rewrap an exception.

[jasontedor]

Thanks @Timshel. Sorry the delay in seeing this one integrated.