Core: Use Shared HttpClientContext to Persist "was-retried" Attribute#13339
Core: Use Shared HttpClientContext to Persist "was-retried" Attribute#13339XJDKC wants to merge 8 commits into
Conversation
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
awaiting UT test as per offline discussion !
| 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 :)
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM !
Thanks @XJDKC ! Nice catch.
| @@ -507,13 +546,18 @@ public static void testHttpMethodOnFailure(HttpMethod method) throws JsonProcess | |||
| // 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.
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
This looks good to me, thanks for validating the issue with non 503 errors as well. I left a few notes on test layout.
|
@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. |
4 participants
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.