Repository navigation
Prevent Retries 5xx Errors for Non-Idempotent Requests - #1
Merged
Merged
Conversation
…ries by throwing CommitStateUnknown (apache#12818)
dcagney
changed the title
dcagney
changed the title
kebab-mai-haddi
self-requested a review
July 8, 2025 16:05
kebab-mai-haddi
approved these changes
Jul 8, 2025
kebab-mai-haddi
left a comment
kebab-mai-haddi
left a comment
There was a problem hiding this comment.
Consider adding logging when retry decisions are made for better debugging.
APPROVED with minor fixes - This is a critical fix that prevents data corruption. The approach of differentiating between idempotent and non-idempotent requests is correct. Please address the duplicate entry and formatting issues before merging.
Comment on lines
+96
to
+100
| HttpStatus.SC_TOO_MANY_REQUESTS, | ||
| HttpStatus.SC_SERVICE_UNAVAILABLE, | ||
| HttpStatus.SC_INTERNAL_SERVER_ERROR, | ||
| HttpStatus.SC_SERVICE_UNAVAILABLE, | ||
| HttpStatus.SC_BAD_GATEWAY, |
There was a problem hiding this comment.
Is this a duplicate?
HttpStatus.SC_SERVICE_UNAVAILABLE,
| } catch (NumberFormatException ignore) { | ||
| Instant retryAfterDate = DateUtils.parseStandardDate(value); | ||
| if (retryAfterDate != null) { | ||
| Instant retryAfterDate = DateUtils.parseStandardDate(value); if (retryAfterDate != null) { |
There was a problem hiding this comment.
improve the formatting here?
Instant retryAfterDate = DateUtils.parseStandardDate(value);
if (retryAfterDate != null) {
Open
3 tasks
davkim
added a commit
that referenced
this pull request
Sep 8, 2026
…tes; extract shared DuplicateFileRegistrationGuard Ports the additional scope #8 built while retargeting this guard to the 1.11 base, back onto this branch: - Extracts the detection/repair logic this branch already had inline in RewriteDataFilesSparkAction into a shared DuplicateFileRegistrationGuard, used by three actions instead of one. remove_dangling_deletes reproduces the delete-side "2-of-2 removal" half of the original incident on its own (DeleteFileSet's identity key is (location, contentOffset, contentSizeInBytes), identical across two live registrations of one physical file), and rewrite_position_delete_files is run standalone from oncall/backfill notebooks with no dependency on rewrite_data_files ever having run -- both needed the identical guard, not a second drifting copy of this logic. - New core test suite, TestDuplicateRegistrationRepair (66 tests): argument validation, the keep-lowest-sequence-number invariant, 3+ registrations collapsing to one, delete-file repair, that repairing a data file doesn't strip its own delete coverage, and retry safety against a concurrent commit changing the intended survivor between scan and commit. - New ManifestFilterManager checks: validateNoContradictoryDuplicateRegistrationIntent (rejects a commit that both keeps a survivor and separately deletes that same path outright -- the two instructions silently resolved in the destructive direction before this), and hoists isDuplicateRegistrationToDrop's two call sites out of their `||` chains into local variables, since Java's short-circuiting could skip the survivor-recording side effect when an earlier term in the same chain already matched the entry. - TestRemoveDanglingDeleteAction: one existing test's fixture (testPartitionedDeletesWithDanglingDvs) incidentally re-registers two data files at a second sequence number, which the guard now repairs before the dangling-delete pass runs -- updated its post-repair assertions accordingly, and added a new test driving the "delete side" defect deliberately rather than incidentally. - TestRewriteDataFilesAction: narrowed testDuplicateDataFileRegistrationCheckCanBeDisabled's assertion to just "the opt-out doesn't throw this guard's exception" -- two Thor runs disagreed on what happens downstream of the opt-out (planner bin-packing determines whether the duplicate lands in a selected file group), so asserting no exception at all was asserting a coin flip. - Fixed a pre-existing checkstyle violation in RemoveDanglingDeletesSparkAction (new ArrayList<>() instead of Lists.newArrayList()) while already touching that file -- present since #4 merged, apparently never caught by CI until now. TestRewritePositionDeleteFilesAction's new testIcebergItselfRefusesToCreateADuplicateDvRegistration had to be adapted rather than ported verbatim: it relied on rewrite_position_delete_files converting V2 position deletes to V3 DVs, a capability this branch's RewritePositionDeleteFilesSparkAction doesn't have (it hard-refuses to run against a V3 table at all here). Generates the DV directly via FileGenerationUtil instead, matching TestRemoveDanglingDeleteAction's existing pattern -- same assertion, without depending on a feature this base doesn't have. Also found and fixed, independent of the port: DuplicateFileRegistrationGuard's validateNoContradictoryDuplicateRegistrationIntent on the 1.11 branch has a comment stating it must read callerDeletedFilePaths (not deleteFiles, which the filtering loop also writes to and never clears between commit attempts) but the code still reads deleteFiles -- this port implements what the comment says, not what the code currently does. Worth a look on #8 too. Tested: TestDuplicateRegistrationRepair (66 tests, 0 failures), TestDuplicateFileRegistrationGuard (5 tests, 0 failures), TestRemoveDanglingDeleteAction (12 tests, 2 skipped, 0 failures), TestRewriteDataFilesAction (full suite, 122 tests, 2 skipped, 0 failures), TestRewritePositionDeleteFilesAction (full suite, 19 tests, 0 failures). spotlessCheck and checkstyle clean on every file this commit touches -- iceberg-core:checkstyleTest still fails on this branch, but only on core/src/test/java/org/apache/iceberg/rest/* (unused imports, annotation style), pre-existing since PR #1 merged and unrelated to this change.
This was referenced Sep 28, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This PR prevents non-idempotent Request Methods (i.e. POSTs) from automatically retrying on 5xx errors.
This prevents retries from corrupting table metadata in the event that a commit is applied and persisted in the catalog BE, but returns a 5xx error. If these requests are retried, subsequently Fail (i.e. with a 409), and attempt to cleanup the files they've written, table metadata will be corrupted.
This PR primarily contains cherry-picks from OSS PRs:
And the following Affirm-authored updates:
No regressions are introduced by these changes, all iceberg-core unit tests that were passing in apache-iceberg-1.8.1 are passing in this patch.
The single failed test, TestHadoopCommits#testConcurrentFastAppends, is also failing on the iceberg 1.8.1 release tag.
