Core: Use Shared HttpClientContext to Persist "was-retried" Attribute #13339
Conversation
| ErrorResponse enrichedErrorResponse = | ||
| ErrorResponse.builder() | ||
| .wasRetried(wasRetried == Boolean.TRUE) | ||
| .wasRetried(Boolean.TRUE.equals(wasRetried)) |
There was a problem hiding this comment.
It's not required, I just got used to using equals after seeing some static analysis warnings. Using == may not pass certain checks, even though it's safe here.
There was a problem hiding this comment.
I was also interested in this change, but if it avoids a warning, sounds good to me :)
| // headers. | ||
| // Return the path generated for the test case, so that the client can call that path to exercise | ||
| // it. | ||
| private static String addRequestTestCaseAndGetPath(HttpMethod method, Item body, int statusCode) |
There was a problem hiding this comment.
Since nothing else broke I think the change here is ok, but we should probably update the doc to note that this will only set the response for the "next" invocation of the path correct?
There was a problem hiding this comment.
Updated the doc, I'll address this more thoroughly in the follow-up refactoring.
In most cases, we should return a consistent response. However, for retry scenarios, we may need finer control over the responses across retries. For example, it may make sense for the final mock request in a retry sequence to return a fixed, constant response.
|
@XJDKC We may be going a different route with this, there were some offline discussions and we are considering just disabling all retries to just avoid this issue entirely and just throw CSU for all 5XX errors |
Got it, so we're planning to retry only when the error code is |
|
Just realized that this PR is to fix the |
|
given that the |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
Summary
Fixes an issue where retry flag
was-retriedwas not accessible after request execution due to context isolation.Previously, requests were executed with a
BasicHttpContextinstance, which caused Apache HttpClient to create internal child contexts. As a result, any attributes (such aswas-retried) set during the retry process were not visible to the caller after the response.This change ensures that a shared
HttpClientContextis used when executing requests, allowing retry-related attribute to persist and be accessible for error handling and observability.Prior work: #12818
Changes
Use
HttpClientContext.create()instead ofBasicHttpContext.Testing
Adding a unit test that simulates a self-conflict table corruption scenario.