Support opt-in idempotency for updateTable in the Iceberg REST catalog#5037
Support opt-in idempotency for updateTable in the Iceberg REST catalog#5037huaxingao wants to merge 2 commits into
Conversation
| if (winner != null | ||
| && EntityIdempotency.hasLiveKey( | ||
| winner.getInternalPropertiesAsMap(), idempotencyKey, clock().instant())) { | ||
| return catalogHandlerUtils().loadTable(baseCatalog, tableIdentifier); |
There was a problem hiding this comment.
nit: this fragment looks identical to lines 1236-1241... Would it be worth introducing a helper method?
There was a problem hiding this comment.
Extracted the duplicated fragment into a replayIfKeyLive(tableIdentifier, key) helper.
| // Same key, different payload: because the key is still live on the entity, the request replays | ||
| // current catalog state and does NOT apply the new value, so "p" stays "first". | ||
| LoadTableResponse replay = | ||
| updateTableProperties(services, tableId, Map.of("p", "second"), IDEMPOTENCY_KEY); |
There was a problem hiding this comment.
nit: this test is not correct from the IRC spec POV: reusing the same idempotency key is allowed only between requests with the same payload 🤷
Would it be possible to check the entity version ID instead?
It is fine as a "white box" test for the current Polaris impl., though.
There was a problem hiding this comment.
Reworked the test to retry with the same key and same payload and assert via the table version (first update advances metadataFileLocation, retry leaves it unchanged).
| * catalog state instead of re-applying the update (which could fail the request's requirements | ||
| * against the already-advanced table). | ||
| */ | ||
| public class EntityIdempotencyUpdateTableTest { |
There was a problem hiding this comment.
Why not add test cases to EntityIdempotencyTest?
There was a problem hiding this comment.
EntityIdempotencyTest only unit-tests the EntityIdempotency helper, which this PR doesn't change. The updateTable logic is in the handler and commit path, so it needs an end-to-end test, this file is just the update counterpart of the existing EntityIdempotencyCreateTableTest.
| - Added an OpenTelemetry event listener for emitting Polaris audit events as OpenTelemetry log records. | ||
| - Added optional `sessionPolicy` field to `SigV4AuthenticationParameters` for catalog federation. When set, the IAM session policy JSON is attached to the STS AssumeRole request, allowing administrators to restrict vended credentials to only the required AWS services and actions (Principle of Least Privilege). | ||
| - Added opt-in idempotency for `createTable` in the Iceberg REST catalog. When enabled via `polaris.idempotency.enabled=true` (default `false`), a client-supplied `Idempotency-Key` header is embedded into the new table entity and committed in the same transaction; a retry carrying the same key within the TTL window (`polaris.idempotency.ttl`, default `PT5M`) replays the original success instead of failing with `AlreadyExists`. | ||
| - Extended opt-in idempotency to `updateTable` in the Iceberg REST catalog. With idempotency enabled, the `Idempotency-Key` supplied on a commit is stamped onto the table entity in the same transaction as the metadata change; a retry carrying the same key within the TTL window replays the original success from current catalog state instead of failing with `CommitFailedException` when the request's requirements no longer match the already-advanced table. |
There was a problem hiding this comment.
Since opt-in idempotency for createTable has not been released yet, it might be worth grouping both under the same changelog item.
There was a problem hiding this comment.
Merged both into a single changelog entry
flyrain
left a comment
There was a problem hiding this comment.
LGTM overall. Thanks @huaxingao
| Map<String, String> internalProperties = storedProperties; | ||
| // setInternalProperties replaces the whole map, and the idempotency window lives only in | ||
| // internal properties (not table metadata), so carry it forward to keep prior keys live | ||
| // across updates even when a given update carries no key of its own. | ||
| String priorWindow = | ||
| entity.getInternalPropertiesAsMap().get(EntityIdempotency.IDEMPOTENCY_KEYS_PROPERTY); | ||
| if (priorWindow != null) { | ||
| internalProperties.put(EntityIdempotency.IDEMPOTENCY_KEYS_PROPERTY, priorWindow); | ||
| } | ||
| if (idempotencyRequestContext.isActive()) { | ||
| // Stamp this update's key atomically with the metadata change, purging expired keys. | ||
| internalProperties = | ||
| EntityIdempotency.recordKey( | ||
| internalProperties, | ||
| idempotencyRequestContext.pendingKey(), | ||
| idempotencyRequestContext.pendingExpiry(), | ||
| Instant.now()); | ||
| } |
There was a problem hiding this comment.
Nit: we add more code into this already long method. Not sure if we can refactor a bit to avoid a ~200 LOC method.
There was a problem hiding this comment.
Pulled the window/key-stamping logic out of doCommit into a small idempotencyInternalProperties helper.
There was a problem hiding this comment.
Pull request overview
This pull request extends the existing entity-internal-properties idempotency mechanism (previously used for Iceberg REST createTable) to also support opt-in idempotency for single-table updateTable, so a retry with the same Idempotency-Key can replay success from current catalog state instead of re-applying an update.
Changes:
- Persist the active
Idempotency-Keyinto the table entity’s internal properties atomically withupdateTablecommits, while carrying forward and purging the existing key window across updates. - Add
updateTablereplay handling in the Iceberg REST handler: if the key is already live on the entity, return current table state (and also retry-as-replay on a same-keyCommitFailedExceptionrace). - Add end-to-end tests for the
updateTableidempotency behavior and update the changelog entry to includeupdateTable.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/EntityIdempotencyUpdateTableTest.java | Adds end-to-end coverage for keyed replay, unkeyed non-idempotent behavior, and different-key behavior for updateTable. |
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java | Ensures idempotency key windows are preserved across updates and records the current key atomically with metadata commits. |
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java | Implements updateTable replay when a live key is present and handles same-key concurrent races via a replay check on CommitFailedException. |
| CHANGELOG.md | Updates the unreleased feature note to reflect opt-in idempotency support for both createTable and updateTable. |
Extends the entity-property (single-transaction) idempotency introduced for
createTable(#4912) to the single-table
updateTablepath.What changed
LocalIcebergCatalog.doCommit, update branch): theIdempotency-Keyis written into the table entity's internal properties in the sametransaction as the metadata change, so the key and the update commit atomically. The prior
key window is carried forward across updates (it lives only in internal properties, which
setInternalPropertieswould otherwise replace), and expired keys are purged inline.IcebergCatalogHandler.updateTable): when the target entity already carries alive copy of the request's key, the handler returns current catalog state instead of
re-applying the update.
CommitFailedExceptionisretried as a replay when a same-key race winner has already committed the key atomically
with its metadata change; otherwise the exception is surfaced as before.
This is behind the existing
polaris.idempotency.enabledflag (defaultfalse) and reusesthe existing key TTL (
polaris.idempotency.ttl, defaultPT5M).Scope / intentional exclusions
updateTableonly, mirroring the focused scope ofcreateTable.deleteis intentionally not covered. The entity-property model stamps the key ontothe entity produced by the operation; a delete removes the entity, so there is nowhere to
record the key within the same transaction.
decode()graceful degradation) are deliberately deferred to a separate clean-up PR tokeep this change focused.
Testing
New
EntityIdempotencyUpdateTableTestcovers:(this also exercises the key-window carry-forward path).
The concurrent-race
CommitFailedExceptionbranch is not unit-tested, as it can't betriggered deterministically; the logic mirrors the reviewed
createTablerace handling.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)