Conversation
|
There was a problem hiding this comment.
Although it's an optional ticket, this will be a reference implementation, so it would be nice if you could implement it.
I suggest using the same messages as .NET SDK for consistency.
However, if you choose not to implement this, we should still add some auto-generated request ID to the debug log messages so the individual requests can be followed when reading the logs (think about situations where multiple SDK instances are running and writing logs to the same output.)
| return this.getResponseAsync(eTag).thenComposeAsync(response -> { | ||
| if (response.shouldRetry()) { | ||
| try { | ||
| this.httpClient.connectionPool().evictAll(); |
There was a problem hiding this comment.
As per the specification in the ticket, we should only reset the connection pool if at least 30 seconds have passed since the last reset. (If there's been no reset yet, we should reset it immediately.)
This is to prevent socket exhaustion, which might otherwise be possible in extreme cases as, according to RFC 9293, TCP connection are kept around for some time in the TIME_WAIT state.
| logger.error(logEventId, message, e); | ||
| } | ||
| future.complete(FetchResponse.failed(message, false, null)); | ||
| future.complete(FetchResponse.failed(message, false, null, true)); |
There was a problem hiding this comment.
Could you ensure that, just like in onResponse, future.complete is called even if logger.error throws an exception (which may be possible as it can execute user-provided code)?
2 participants
Describe the purpose of your pull request
Related issues (only if applicable)
How to test? (only if applicable)
Security (only if applicable)
Requirement checklist (only if applicable)