flyrain commented on code in PR #4659: URL: https://github.com/apache/polaris/pull/4659#discussion_r3432391458
########## persistence/relational-jdbc/src/main/resources/cockroachdb/schema-v5.sql: ########## @@ -0,0 +1,303 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file-- +-- distributed with this work for additional information +-- regarding copyright ownership. The ASF licenses this file +-- to you under the Apache License, Version 2.0 (the +-- "License"). You may not use this file except in compliance +-- with the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, +-- software distributed under the License is distributed on an +-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +-- KIND, either express or implied. See the License for the +-- specific language governing permissions and limitations +-- under the License. + +-- CockroachDB schema v4 (matching PostgreSQL schema v4) Review Comment: Header says "CockroachDB schema v4 (matching PostgreSQL schema v4)" but this is the v5 file — fix the label. ########## 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: Should we persist idempotency key and table creation in the same transaction? I'm bit worry about the correctness issue, for example, if create table succeeded, but idempotency key persistence failed. Polaris will response to client with a table create failure, right? -- 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]
