dimas-b commented on code in PR #4659:
URL: https://github.com/apache/polaris/pull/4659#discussion_r3456369241
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -460,14 +485,98 @@ public void authorizeCreateTableDirect(
}
}
+ /**
+ * Create a table, optionally honoring an {@code Idempotency-Key} from the
REST request.
+ *
+ * <p>When an idempotency key is supplied and the feature is enabled:
+ *
+ * <ol>
+ * <li>Authorization runs first, so idempotency cannot bypass it.
+ * <li>Pre-flight loads any prior record for {@code (realm, key)}. A match
(same caller, same
+ * resource binding) replays the response from authoritative catalog
state; a binding
+ * mismatch raises 422.
+ * <li>On a fresh key, the table is created and the record is inserted
afterwards. A concurrent
+ * caller that wins the race causes our insert to return DUPLICATE —
we then replay too, so
+ * the response is equivalent to what the winner returned.
+ * </ol>
+ *
+ * <p>No response body is stored. Replays go through {@code loadTable +
+ * buildLoadTableResponseWithDelegationCredentials}, which re-vends fresh
credentials for the
+ * current caller.
+ */
public LoadTableResponse createTableDirect(
Namespace namespace,
CreateTableRequest request,
EnumSet<AccessDelegationMode> delegationModes,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ Optional<UUID> idempotencyKey) {
authorizeCreateTableDirect(namespace, request, !delegationModes.isEmpty());
Optional<AccessDelegationMode> resolvedMode =
resolveAccessDelegationModes(delegationModes);
+ TableIdentifier tableIdentifier = TableIdentifier.of(namespace,
request.name());
+
+ // Pre-flight owns the full idempotency decision: it returns Disabled when
the feature is off or
+ // no key was supplied (plain create path), Duplicate to replay a prior
success, or Owned to
+ // proceed. Authorization already ran above, so a replay reloads current
catalog state (no
+ // response body is stored) and re-vends credentials for this caller;
+ // buildLoadTableResponseForExistingTable raises 422 if the table has
advanced beyond the
+ // metadata location captured when the key was recorded. A binding
mismatch surfaces as
+ // IdempotencyConflictException, which PolarisExceptionMapper maps to HTTP
422.
+ IdempotencyOutcome preflight =
+ idempotencySupport()
+ .preflight(
+ idempotencyKey,
+ polarisPrincipal(),
+ IdempotentOperation.CREATE_TABLE,
+ namespace.toString(),
+ request.name(),
+ resolvedMode.map(Enum::name).orElse("none"));
+ if (preflight instanceof IdempotencyOutcome.Duplicate(IdempotencyRecord
existing)) {
+ return buildLoadTableResponseForExistingTable(
+ tableIdentifier,
+ resolvedMode,
+ CREATE_TABLE_STORAGE_ACTIONS,
+ refreshCredentialsEndpoint,
+ existing.metadataLocation());
+ }
+
+ // Run the operation. A concurrent request carrying the same key can win
the catalog-level race
+ // and make this attempt fail with AlreadyExistsException; if that winner
recorded a matching
+ // idempotency outcome, replay it instead of returning a 409.
+ LoadTableResponse response;
+ try {
+ response = doCreateTableDirect(namespace, request, resolvedMode,
refreshCredentialsEndpoint);
+ } catch (AlreadyExistsException e) {
+ Optional<IdempotencyRecord> raceWinner =
+ idempotencySupport().resolveConcurrentDuplicate(preflight);
+ if (raceWinner.isPresent()) {
+ return buildLoadTableResponseForExistingTable(
+ tableIdentifier,
+ resolvedMode,
+ CREATE_TABLE_STORAGE_ACTIONS,
+ refreshCredentialsEndpoint,
+ raceWinner.get().metadataLocation());
+ }
+ // Not a same-key retry: the table genuinely pre-existed, so this is a
real conflict.
+ throw e;
+ }
+
+ // Record the successful outcome. recordOutcome is a no-op unless
idempotency is in effect, and
+ // throws (422) if the key was reused with a different binding. A
concurrent same-key,
+ // same-table
+ // create cannot also reach this point — the loser fails the catalog
create above and replays
+ // via
+ // resolveConcurrentDuplicate — so there is no winner-recorded-first case
to replay here.
+ String metadataLocation = response.tableMetadata().metadataFileLocation();
+ idempotencySupport().recordOutcome(preflight, 200, metadataLocation);
Review Comment:
This is a significant change in the overall feature design. I think this
needs re-opening the end-to-end design discussion on `dev`.
IIRC, currently transaction boundaries are expected to be at each
Persistence SPI method call level.
Cf. https://lists.apache.org/thread/rf5orxs815zs4h64p4rwp03q3pbgxb5r
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
