REST: Revert #12818 and additionally stop retrying on 502/504 #13352
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 5xx retries by throwing CommitStateUnknown (apache#12818)" This reverts commit 4927610.
Contributor
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @singhpk234 this looks good just had a minor comment on testing that would be good to address
core/src/test/java/org/apache/iceberg/rest/TestExponentialHttpRequestRetryStrategy.java
Outdated
Show resolved
Hide resolved
amogh-jahagirdar
changed the title
singhpk234
force-pushed
the
fix/irc-client-retries
branch
from
41ade9a to
34f57db
Compare
amogh-jahagirdar
approved these changes
Jun 19, 2025
Contributor
There was a problem hiding this comment.
Thanks @singhpk234 this looks great now! I'll hold off before merging for @RussellSpitzer @rdblue feedback
nastra
reviewed
Jun 24, 2025
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java
Show resolved
Hide resolved
RussellSpitzer
approved these changes
Jul 1, 2025
Member
|
LGTM, @dennishuo has now scared me of 503's and anonymous 409's (not thrown by the IRC) potentially causing issues but I think we can address that in a follow up if we get consensus. |
Member
|
Thanks @singhpk234 for reverting and fixing again! Thanks @amogh-jahagirdar, @nastra , @dennishuo and @rdblue for discussion and review |
singhpk234
added a commit
to singhpk234/iceberg
that referenced
this pull request
Jul 3, 2025
singhpk234
added a commit
to singhpk234/iceberg
that referenced
this pull request
Jul 3, 2025
RussellSpitzer
pushed a commit
that referenced
this pull request
Jul 3, 2025
dcagney
pushed a commit
to Affirm/iceberg
that referenced
this pull request
Jul 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About the change
presently, we retry on 502 / 504 as well here, we have a spec definition stating here that when these status are thrown the commit status can be unknown, so says the RFC as something went wrong in the middle of processing the request. So we retry with 502 and 504 we can conflict with ourselves, as we don't know when we receive the status of 409 is it because we retried on 502 and then got 409 or something else happened, so it's better to throw the commit state unknown if we know the commit has been retried.
The Iceberg users who complained this was with 504 : slack thread - https://apache-iceberg.slack.com/archives/C025PH0G1D4/p1747992294134219
We have similar handling for glue as we did here #7198 as there are internal http retries over the glue SDK.
So I Added a PR considering above #12818
But based on offline discussions with more folks, according to the spec its best to not retry on 502 / 504 as it clearly calls out that the status is unknown.
The change attempts to :
Testing
Adding unit tests
cc @amogh-jahagirdar @rdblue @RussellSpitzer