Add handler-level Idempotency-Key support for createTableDirect by huaxingao · Pull Request #4269 · apache/polaris

huaxingao · 2026-04-22T03:23:20Z

Adds handler-level Idempotency-Key support for createTableDirect

Runs after authorization (no auth bypass).
Binding includes the caller's principal hash
Replay re-vends fresh credentials via the normal vending path; no
credentials are stored.

createTableStaged, other mutations, and benchmarking are deferred to
follow-up issues.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

huaxingao · 2026-04-29T22:16:12Z

    String idempotencyKey,
    String operationType,
    String normalizedResourceId,
+    String principalHash,


Added principal_hash for caller-identity binding and dropped response_headers since credential-bearing mutations re-vend on replay rather than replaying stored response data.

Since this class existed before, and current PR concentrates on integrating idempotency support into Polaris services, would you mind extracting functional changes into a new PR to be merged before this PR?

I believe this will make reviews easier by isolating functional changes from infrastructural changes.

huaxingao · 2026-04-29T22:16:15Z

-   * applicable).
-   */
-  @Override
-  public String normalizedResourceId() {


Removed these methods — since this is a record, the compiler already auto-generates identical accessors; the hand-written @OVERRIDES just shadowed them.

huaxingao · 2026-04-29T22:31:24Z

+   *
+   * @return {@code true} if the reservation was removed, {@code false} otherwise
+   */
+  boolean cancelInProgressReservation(String realmId, String idempotencyKey, String executorId);


New method to release the idempotency key when the operation fails before any state is committed

singhpk234 · 2026-04-30T04:46:57Z

+   */
+  public String principalHash(String principalName, String realmId) {
+    return sha256Hex(principalName + ":" + realmId);
+  }


This is incorrect, we need the whole principal obj to do proper AuthZ

Fixed. Thanks!

flyrain · 2026-05-01T18:15:55Z

+   * response with freshly-vended credentials for the current caller. No credentials from the
+   * original call are stored or returned.
+   */
+  private LoadTableResponse replayCreateTableDirect(


Worth a one-line comment here explaining why this path doesn't re-authorize: authorizeCreateTableDirect at line 508 already ran for this same principalHash, which is why replay via loadTable is safe without running load-table authz. Makes the invariant explicit for future readers.

Added more comments in the java doc to explain.

flyrain · 2026-05-01T18:15:55Z

+      // The original create succeeded but the table is no longer there (manual drop, retention,
+      // etc.). Surface the same not-found the client would get from a normal load.
+      throw e;
+    }


The try { … } catch (NoSuchTableException e) { throw e; } is dead — the catch just rethrows. Drop the try/catch; the comment about "original succeeded but table later dropped" can stay above the bare baseCatalog.loadTable(...) call.

Removed the no-op try/catch

flyrain · 2026-05-01T18:15:55Z

+            "Timed out waiting for in-progress idempotency key to finalize");
+      }
+      try {
+        Thread.sleep(Math.max(1L, interval.toMillis()));


This blocks the Quarkus HTTP worker thread, and the default inProgressWait is PT30S — a caller who guesses an in-use key can pin a worker for 30s. Either lower the default, cap the total per-request wait aggressively, or move the wait off the request thread. At minimum, call out the worker-pool implication in IdempotencyConfiguration.inProgressWait() javadoc and require route-level rate limiting.

Lowered the default wait to 2s. Also added a Javadoc note saying this wait runs on the worker thread, so operators should keep it small and cap how many requests can hit idempotent endpoints at once.

flyrain · 2026-05-01T20:12:57Z

+    new TreeSet<>(principal.getRoles()).forEach(r -> sb.append(r).append(','));
+    sb.append('|');
+    sb.append("props=");
+    new TreeMap<>(principal.getProperties())


Consider dropping properties from the hash. I think it's fragile to have them in the hash. Two problems:

Admin-driven fragility. PolarisServiceImpl.updatePrincipal (line 316) lets an admin
mutate a principal's properties at any time. TTL defaults to PT5M, so any unrelated
property edit during that window — SCIM sync, display-name update, tagging — breaks
the retry from the same client with the same identity: principalHash no longer matches,
reservation looks "used by a different caller", the client gets 400/422 and has to start
over. No auth-relevant thing changed.

No security upside. (name, realm, roles) should be good enough. Properties are decoration,
not authentication context.

dropped properties

flyrain

Thanks @huaxingao for working on it. I think we are getting close.

flyrain · 2026-05-04T22:30:48Z

    IdempotencyRecord rec = r2.existing().get();
    assertThat(rec.operationType()).isEqualTo(op1);
    assertThat(rec.normalizedResourceId()).isEqualTo(rid1);
+    assertThat(rec.principalHash()).isEqualTo("principal-hash-A");


one more case worth adding here: two reservations with the same (realm, key) but different principalHash. This test covers mismatched op/resource, but the cross-principal path is the core security property of the v5 change and isn't directly exercised at the persistence layer.

Added a new test

flyrain · 2026-05-04T22:46:02Z

+  // ============================================================================
+  // Handler-level idempotency (tenant-visible behaviour knobs)
+  // ============================================================================
+  //
+  // Platform/infrastructure knobs (keyHeader, executorId, purgeExecutorId, polling interval,
+  // purge interval, purge grace) live in IdempotencyConfiguration as @ConfigMapping deploy
+  // constants. The values below are the knobs that operators may legitimately want to vary
+  // per-realm or even per-catalog — for example, enabling idempotency for one catalog while
+  // leaving it off globally, or tuning the in-progress wait for a specific tenant.


It may not be quite valuable to add comments here, as all configs in this class should be tenant-visible. I'd suggest to add it to user-face doc if needed. I'm also OK to remove it.

flyrain · 2026-05-05T01:05:58Z

+  public static final FeatureConfiguration<Boolean> IDEMPOTENCY_ENABLED =
+      PolarisConfiguration.<Boolean>builder()
+          .key("IDEMPOTENCY_ENABLED")
+          .catalogConfig("polaris.config.idempotency.enabled")


Looks like we only make this changeable per-catalog, the followup configs(like IDEMPOTENCY_TTL_GRACE_MILLIS) are per-realm only. Any specific design consideration for that?

IDEMPOTENCY_ENABLED is per-catalog because it's common to roll out a new feature one catalog at a time. The others don't need to be at the catalog level.

flyrain · 2026-05-05T01:11:49Z

+          .description(
+              "Default TTL (in milliseconds) for newly reserved idempotency keys. After this "
+                  + "duration the reservation may be purged by the background maintenance task.")
+          .defaultValue(5L * 60L * 1000L) // 5 minutes


Just wondering, by making it configurable per-realm, would that cause the maintenance job hard to implement? Maybe not, I think expires_at field would be used for purging, which should be fine.

Yes, you're right: the purge job only looks at each row's expires_at, which is stamped when the reservation is created. It never reads the realm's TTL, so per-realm TTL doesn't complicate maintenance.

flyrain · 2026-05-05T01:23:05Z

+ * than the configured interval.
+ */
+@ApplicationScoped
+public class IdempotencyMaintenance {


I think the default triggering frequency(every minute) may be too high. Imaging every pod will wake up and purge records. I think it is even OK we purge it every 1 hour or so. An alternative way is to decide based on number of expired items.

flyrain · 2026-05-05T01:47:55Z

+      // Cross-principal or cross-binding reuse of the same Idempotency-Key: the request is
+      // well-formed but cannot be processed against the existing reservation. Surface as 422.
+      throw new UnprocessableEntityException("%s", e.getMessage());
+    } catch (IdempotencyHandlerSupport.InProgressTimeoutException e) {


how does heartbeat work? Looks like the method updateHeartbeat() is never called from the production code. It is only called from the tests.

updateHeartbeat() is for handlers that may run longer than LEASE_TTL_MILLIS (default 25s). They periodically extend their lease so duplicate callers know the owner is still alive and won't take over. It's currently not used because createTableDirect is synchronous and finishes in milliseconds — well under the lease — so it just calls finalizeOwned on success or cancelOwned on error. The code is there for future long-running handlers (multi-step or async operations) that need to extend the lease while still working.

dimas-b

Posting some preliminary comments

dimas-b · 2026-05-05T16:40:26Z

+          .defaultValue(false)
+          .buildFeatureConfiguration();
+
+  public static final FeatureConfiguration<Long> IDEMPOTENCY_TTL_MILLIS =


Why do we need a feature flag for TTL? Why not use regular config?... cf. RelationalJdbcConfiguration

We want to separate the configurations: deploy-only platform knobs (header name, executor id, polling/purge intervals) stay on ConfigMapping, while tenant-visible behavior knobs like TTL are exposed as FeatureConfiguration.

I believe this approach overloads FeatureConfiguration unnecessarily.

It is possible to provide realm-specific configuration similar to AuthenticationConfiguration.realms().

Moved them off FeatureConfiguration into IdempotencyConfiguration as system-wide settings

dimas-b · 2026-05-05T16:41:58Z

 */
-public interface BasePersistence extends PolicyMappingPersistence, MetricsPersistence {
+public interface BasePersistence
+    extends PolicyMappingPersistence, MetricsPersistence, IdempotencyPersistence {


I hope we can have a more modular design where BasePersistence does not have to extend IdempotencyPersistence.

That is to say, conceptually, IdempotencyPersistence should not have to be implemented by all Persistence implementations (e.g. if they do not wish to do so).

The methods on IdempotencyPersistence all have default implementations that throw UnsupportedOperationException, so a backend that doesn't want to support idempotency just inherits those defaults and doesn't have to implement anything. The feature is also off by default behind IDEMPOTENCY_ENABLED, so those methods never get called unless someone enables it. Including it in BasePersistencefollows the same pattern as PolicyMappingPersistence, which is also composed in this way.

Having default methods does not negate the fact that BasePersistence is a sub-class of IdempotencyPersistence.

This creates a "funnel" where all functionality is essentially forced into BasePersistence.

From my POV it would be preferable to isolate different persistence areas (metastore and idempotency in this case) and have callers use independent interfaces for each use case.

Implementations can support many/all of persistence-related SPI interfaces in the same class, of course.

To clarify: I believe it is possible to inject IdempotencyPersistence implementations into callers even if BasePersistence does not implement IdempotencyPersistence directly.

dimas-b · 2026-05-05T16:51:46Z

      String executorId,
-      Instant now);
+      Instant now) {
+    throw new UnsupportedOperationException(


Related to the comment about BasePersistence: In an SPI (such as this) having default methods that throw UnsupportedOperationException is not nice from the design POV, IMHO.

If an implementation has to provide this method, it should not be default. If an impl. does not have to provide this method, the default method should not throw 🤔

The default methods are throwing here because idempotency is opt-in, gated by IDEMPOTENCY_ENABLED (off by default). Backends that don't enable it never reach these methods, so the throws are a safety net for misconfiguration, not a normal path.

This also matches an existing pattern: PolicyMappingPersistence is composed into BasePersistence the same way, with several methods that have throwing defaults. I'd prefer to stay consistent. Happy to revisit the broader SPI shape in a follow-up if we want to clean it up across the board.

"Existing" is not always "ideal" 😉

Re: follow-up, I personally do not see a reason to propagate a particular pattern in a new feature if we agree to redo / refactor that pattern soon. Why not implement the new feature following the new pattern and adjust the old code in a follow-up?

I'll open a separate refactor PR.

Even in this PR, the default qualifier does not appear to be necessary... Removing it would resolve my concern 🙂

huaxingao · 2026-05-12T17:39:06Z

@dimas-b Do you have any more comments on this PR? If not, can we merge this one first, and then I will change #4397 accordingly? Or if you can review/merge #4397 first, I will rebase this PR. Thanks

dimas-b · 2026-05-12T23:10:42Z

@huaxingao : my comments above still stand. I believe can do better in this PR, which is why I'm not approving.

Merging #4397 first would work fine, but it might be a rather big effort end-to-end.

As far as this PR is concerned, from my POV it would be sufficient to remove IdempotencyPersistence from BasePersistence and implement related factory changes for proper wiring. I do believe this would be a smaller change but it would move in the same direction as #4397.

huaxingao · 2026-05-13T01:06:54Z

Thanks @dimas-b! I went with your suggestion. Pushed an update that:

Removes IdempotencyPersistence from BasePersistence's extends list.
Adds getOrCreateIdempotencyPersistence to MetaStoreManagerFactory, implemented per backend (JDBC returns the JdbcBasePersistenceImpl, in-memory returns a per-realm InMemoryIdempotencyPersistence, NoSQL throws UnsupportedOperationException).
IdempotencyHandlerSupport and IdempotencyMaintenance now go through the new typed factory accessor instead of getOrCreateSession.

This stays in the same direction as #4397 but is scoped narrowly to the idempotency SPI. PTAL. Thanks!

dimas-b · 2026-05-13T15:30:44Z

+   * (e.g. an in-memory store for dev mode). Backends that do not support handler-level idempotency
+   * should throw {@link UnsupportedOperationException}.
+   */
+  IdempotencyPersistence getOrCreateIdempotencyPersistence(RealmContext realmContext);


Let's introduce a separate factory for IdempotencyPersistence.

I do believe it is functionally not related to the "Metastore" Manager.

cc @dennishuo

FYI: I also added an agenda item for this in the Community Sync call on May 14.

After today's Community Sync discussion, I do not think this is a blocker.

I still think we can have cleaner SPI definitions between this factory and persistence interfaces, but that can be done later.

+1, we might be missing a concrete stance on what we even mean by "Metastore" ;)

On the one hand, we could take it to mean specifically the single physical backend that stores BasePersistence concerns (PolarisEntity).

On the other hand, it could mean "the bag of all persistence-related things".

At least starting with just multiplexing out of here isn't a one-way door, so I agree with iterating on cleaner SPI definitions in a followup

dimas-b · 2026-05-13T15:34:09Z

-import org.apache.polaris.persistence.relational.jdbc.models.Converter;
-import org.apache.polaris.persistence.relational.jdbc.models.ModelIdempotencyRecord;
-
-public class RelationalJdbcIdempotencyStore implements IdempotencyStore {


I actually think it is good to keep idempotency code in a separate class and avoid mixing it into JdbcBasePersistenceImpl.

This may become more relevant if my point about having a separate IdempotencyPersistence factory is accepted.

For easy discoverability/posterity, here's the link to the mailing list discussion that included discussion about the SPI: https://lists.apache.org/thread/nk49rfgbzt2nsvplny14wrp98occ61w4 (and specifically that's the link to my stance).

The TL;DR of my stance was that I agree with wanting to keep IdempotencyPersistence split out of BasePersistence, and basically we can follow the pattern of how IntegrationPersistence was mixed in, still allowing expressing a concrete implementation that explicitly relies on transactions across the persistence interfaces -- today that's expressed via TransactionalPersistence extends BasePersistence, IntegrationPersistence[, IdempotencyPersistence].

Specifically, we wanted the ability for implementors to choose their cross-concern transactionality semantics.

So, if they write a MyBasicPersistenceOnDynamo implements BasePersistence and YoloPersistenceAddonsOnRedis implements IntegrationPersistence, IdempotencyPersistence` that's still possible.

On this line of code though it would mean we're allowing JDBC to have one conceptual concrete implementation which intentionally simultaneously implements the different persistences at once. I suppose we could always still refactor into different JdbcIdempotencyStoreImpl, JdbcIntegrationStoreImpl, etc., but that'd need to have some more complex wiring/shared-state nonetheless if the intent for JDBC specifically is indeed to say that we're intentionally putting them all in the same underlying store.

My point that co-locating Idempotency methods in the same class as MetaStore methods for JDBC does not help or harm transactional processing. What matters is the JDBC Connection (which carries the Tx state).

In this PR, the Idempotency reserve method (as an example), uses a separate Connection and commits before exiting. With this approach it might as well exist in a separate class (to avoid bloating one class with all new functionality).

That said, removing RelationalJdbcIdempotencyStore is not a blocker for this PR from my POV.

dimas-b · 2026-05-13T15:37:15Z

-      case POSTGRES -> 4; // PostgreSQL has schemas v1, v2, v3, v4
-      case COCKROACHDB -> 4; // CockroachDB schema version kept in sync with PostgreSQL
-      case H2 -> 4; // H2 uses same schemas as PostgreSQL
+      case POSTGRES -> 5; // PostgreSQL has schemas v1, v2, v3, v4, v5


This is ok, but it might be best to wait until 1.5.0 is branched to avoid major schema changes just before the release... WDYT?

dimas-b · 2026-05-13T15:40:23Z

    String idempotencyKey,
    String operationType,
    String normalizedResourceId,
+    String principalHash,


Since this class existed before, and current PR concentrates on integrating idempotency support into Polaris services, would you mind extracting functional changes into a new PR to be merged before this PR?

I believe this will make reviews easier by isolating functional changes from infrastructural changes.

dimas-b · 2026-05-13T15:42:18Z

      String executorId,
-      Instant now);
+      Instant now) {
+    throw new UnsupportedOperationException(


Even in this PR, the default qualifier does not appear to be necessary... Removing it would resolve my concern 🙂

dimas-b · 2026-05-13T15:45:22Z

+  public synchronized IdempotencyPersistence getOrCreateIdempotencyPersistence(
+      RealmContext realmContext) {
+    return idempotencyPersistenceMap.computeIfAbsent(
+        realmContext.getRealmIdentifier(), k -> new InMemoryIdempotencyPersistence());


I'm not sure this is correct. A LocalPolarisMetaStoreManagerFactory may have sub-classes that are not "in memory" (e.g. in downstream projects).

While overriding this method is possible in such cases, the default behaviour would not be correct.

This is another point for introducing a separate factory for IdempotencyPersistence.

dimas-b · 2026-05-13T15:46:59Z

+/**
+ * SPI conformance tests for {@link IdempotencyPersistence} implementations against the in-memory
+ * implementation. The same scenarios are covered against the JDBC implementation in {@code
+ * RelationalJdbcIdempotencyPersistencePostgresIT}.


RelationalJdbcIdempotencyPersistencePostgresIT does not exist, does it?

dimas-b · 2026-05-13T15:48:25Z

+
+/**
+ * SPI conformance tests for {@link IdempotencyPersistence} implementations against the in-memory
+ * implementation. The same scenarios are covered against the JDBC implementation in {@code


Is this is meant to be a conformance test suite, should it not be shared among backend-specific implementations (rather than copied)?

dimas-b · 2026-05-13T15:55:48Z

+
+  /** Returns true if handler-level idempotency is enabled. */
+  public boolean isEnabled() {
+    return configuration.enabled();


I believe it is preferable to control idempotency-related call paths in a way that takes actual java code into account. For example, if the idempontency SPI is not implemented, it should not be called even if configuration enables it.

This can be achieved via a dedicated factory (injected by CDI). The factory could provide a this flag in a way consistent with impl. code.

Configuration would be done in a way similar to PolarisAuthorizerFactory.

Following up on today's Community Sync discussion, would it make sense to an .enabled() method to IdempotencyPersistence instead of this config flag?

huaxingao · 2026-05-13T16:56:08Z

Thanks @dimas-b very much for your review! I will address these comments shortly.

dimas-b · 2026-06-12T15:27:35Z

Is this PR superseded by #4659 ?

huaxingao · 2026-06-12T21:55:08Z

superseded by #4659

github-project-automation Bot added this to Basic Kanban Board Apr 22, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Apr 22, 2026

huaxingao added 4 commits April 28, 2026 21:40

Add handler-level Idempotency-Key support for createTableDirect

74559ae

Fix invalid Duration default for idempotency in-progress poll interval

7a33c8b

fix test failure

9d3be53

Trigger CI

e4a1a91

huaxingao force-pushed the idempotency-handler branch from 473cdb2 to e4a1a91 Compare April 29, 2026 04:40

huaxingao commented Apr 29, 2026

View reviewed changes

singhpk234 reviewed Apr 30, 2026

View reviewed changes

Include principal roles and properties in idempotency hash

5a606e1

flyrain reviewed May 1, 2026

View reviewed changes

huaxingao added 3 commits May 2, 2026 09:52

address comments

de532e2

fix test

bc0a02c

regenerate idempotency config docs

66f7418

flyrain reviewed May 5, 2026

View reviewed changes

huaxingao added 2 commits May 4, 2026 23:44

address comments

628f07f

fix

97c3207

dimas-b reviewed May 5, 2026

View reviewed changes

huaxingao added 4 commits May 5, 2026 11:02

address comments

de037f1

Move idempotency settings to IdempotencyConfiguration

94a439d

spotlessApply

1453eb8

Regenerate config docs

7feb581

Decouple IdempotencyPersistence from BasePersistence

d2a404f

dimas-b mentioned this pull request May 13, 2026

Core: Isolate MetricsPersistence from BasePersistence SPI #4397

Merged

6 tasks

dimas-b reviewed May 13, 2026

View reviewed changes

huaxingao mentioned this pull request May 14, 2026

Extract IdempotencyRecord data-model change and JDBC schema v5 #4431

Closed

6 tasks

huaxingao closed this Jun 12, 2026

github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Jun 12, 2026

Conversation

huaxingao commented Apr 22, 2026

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dimas-b May 13, 2026 •

edited

Loading

dimas-b May 5, 2026 •

edited

Loading

dimas-b May 5, 2026 •

edited

Loading