From 9a1c377234973cef668bf14664079e695c83520c Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Sat, 10 Oct 2026 19:17:14 +0800 Subject: [PATCH 1/9] [rest] Add table commit lease protocol --- docs/docs/concepts/concurrency-control.md | 23 +++ docs/generated/core_configuration.html | 6 + docs/static/rest-catalog-open-api.yaml | 131 ++++++++++++++++++ .../java/org/apache/paimon/CoreOptions.java | 10 ++ .../java/org/apache/paimon/rest/RESTApi.java | 22 +++ .../org/apache/paimon/rest/ResourcePaths.java | 4 + .../rest/requests/CommitLockRequest.java | 52 +++++++ .../rest/responses/CommitLockResponse.java | 90 ++++++++++++ .../org/apache/paimon/catalog/Catalog.java | 21 +++ .../paimon/catalog/CatalogCommitLock.java | 54 ++++++++ .../paimon/catalog/DelegateCatalog.java | 13 ++ .../org/apache/paimon/rest/RESTCatalog.java | 36 +++++ .../paimon/rest/CommitLockProtocolTest.java | 126 +++++++++++++++++ 13 files changed, 588 insertions(+) create mode 100644 paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java create mode 100644 paimon-api/src/main/java/org/apache/paimon/rest/responses/CommitLockResponse.java create mode 100644 paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java create mode 100644 paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index cdc1f85d4a15..18c076a106f1 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -72,6 +72,29 @@ The commit mechanism depends on the catalog and storage: All writers of the same table must use a compatible commit mechanism and shared locking configuration. See [Catalog](./catalog) when choosing the metadata backend. +## REST Catalog Commit Leases + +A REST catalog can expose optional table commit leases. Set `rest.commit.lock-enabled = true` +on the table to allow lease acquisition; the option defaults to `false`. Enabling this capability +does not automatically make writers acquire leases. The catalog API exposes acquisition and +renewal for callers that manage a commit lease explicitly. + +The client supplies the stable table UUID and its exact `commitUser` when acquiring and renewing. +A grant includes the server's current snapshot; the holder uses that head to rebuild and validate +its changes, renewing the lease while preparing and publishing. The existing snapshot commit +request is unchanged: the server checks ownership using `snapshot.commitUser` and the authenticated +caller. It must enforce this admission check for all writers in the same transaction as publication, +including writers that do not acquire leases. A new successful publication clears the lease +atomically. Abandoned attempts stop renewal and expire automatically; there is no explicit unlock API. + +This is a commit admission lease, not a generation or fencing token. Requests with the same +`commitUser` and authenticated caller share ownership. Use a unique commit user per logical writer +and preserve it during recovery. Snapshot UUID comparison and file conflict validation remain +required; leases cannot make stale file changes valid or guarantee success after lease loss. + +This REST table capability is independent of the catalog options `lock.enabled` and `lock.type`, +which configure the existing catalog lock used for filesystem publication. + ## Files conflict A writer validates file-level changes as well as the snapshot ID. For example, if two compactors diff --git a/docs/generated/core_configuration.html b/docs/generated/core_configuration.html index a1c67478ac1d..36ad11bfbb44 100644 --- a/docs/generated/core_configuration.html +++ b/docs/generated/core_configuration.html @@ -1470,6 +1470,12 @@ String Time field for record level expire. It supports the following types: `timestamps in seconds with INT`,`timestamps in seconds with BIGINT`, `timestamps in milliseconds with BIGINT` or `timestamp`. + +
rest.commit.lock-enabled
+ false + Boolean + Enable the REST catalog's table commit lease capability. +
row-tracking.enabled
false diff --git a/docs/static/rest-catalog-open-api.yaml b/docs/static/rest-catalog-open-api.yaml index a52be685d04c..d3cd92cb6fd1 100644 --- a/docs/static/rest-catalog-open-api.yaml +++ b/docs/static/rest-catalog-open-api.yaml @@ -615,6 +615,104 @@ paths: $ref: '#/components/responses/TableNotExistErrorResponse' "500": $ref: '#/components/responses/ServerErrorResponse' + /v1/{prefix}/databases/{database}/tables/{table}/commit-lock: + post: + tags: + - table + summary: Acquire a table commit lease + operationId: acquireCommitLock + parameters: + - name: prefix + in: path + required: true + schema: + type: string + - name: database + in: path + required: true + schema: + type: string + - name: table + in: path + required: true + schema: + type: string + description: >- + Requires rest.commit.lock-enabled=true and the same permission as committing snapshots. + Ownership is the exact commitUser together with the authenticated caller. Contention + returns HTTP 200 with acquired=false. A grant includes the authoritative snapshot head. + Renewal never revives an expired lease. Publication retains snapshot CAS validation + and atomically releases the lease on a new successful commit. There is no unlock RPC. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/CommitLockRequest' + responses: + "200": + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/CommitLockResponse' + "400": + $ref: '#/components/responses/BadRequestErrorResponse' + "401": + $ref: '#/components/responses/UnauthorizedErrorResponse' + "404": + $ref: '#/components/responses/TableNotExistErrorResponse' + "500": + $ref: '#/components/responses/ServerErrorResponse' + /v1/{prefix}/databases/{database}/tables/{table}/commit-lock/renew: + post: + tags: + - table + summary: Renew a table commit lease + operationId: renewCommitLock + parameters: + - name: prefix + in: path + required: true + schema: + type: string + - name: database + in: path + required: true + schema: + type: string + - name: table + in: path + required: true + schema: + type: string + description: >- + Requires rest.commit.lock-enabled=true and the same permission as committing snapshots. + Ownership is the exact commitUser together with the authenticated caller. Contention + returns HTTP 200 with acquired=false. A grant includes the authoritative snapshot head. + Renewal never revives an expired lease. Publication retains snapshot CAS validation + and atomically releases the lease on a new successful commit. There is no unlock RPC. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/CommitLockRequest' + responses: + "200": + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/CommitLockResponse' + "400": + $ref: '#/components/responses/BadRequestErrorResponse' + "401": + $ref: '#/components/responses/UnauthorizedErrorResponse' + "404": + $ref: '#/components/responses/TableNotExistErrorResponse' + "500": + $ref: '#/components/responses/ServerErrorResponse' /v1/{prefix}/databases/{database}/tables/{table}/rollback: post: tags: @@ -3499,6 +3597,39 @@ components: $ref: '#/components/schemas/Identifier' destination: $ref: '#/components/schemas/Identifier' + CommitLockRequest: + type: object + required: [tableId, commitUser] + properties: + tableId: + type: string + description: Stable resolved table or branch UUID; a recreated table cannot inherit a lease. + commitUser: + type: string + minLength: 1 + maxLength: 255 + description: Exact commitUser that will appear in the submitted snapshot. + CommitLockResponse: + type: object + required: [acquired, expiresAtMillis, leaseMillis] + properties: + acquired: + type: boolean + commitUser: + type: [string, "null"] + expiresAtMillis: + type: integer + format: int64 + description: Server database clock expiration; zero when acquired is false. + leaseMillis: + type: integer + format: int64 + description: Renewal duration in milliseconds; zero when acquired is false. + snapshot: + anyOf: + - $ref: '#/components/schemas/Snapshot' + - type: "null" + description: Authoritative head at grant time; null for an empty table. CommitTableRequest: type: object properties: diff --git a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java index 98f0dfe048e6..1b6080148e57 100644 --- a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java +++ b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java @@ -973,6 +973,12 @@ public InlineElement getDescription() { "Specify how to initialize the next sequence number for primary key " + "table writers."); + public static final ConfigOption REST_COMMIT_LOCK_ENABLED = + key("rest.commit.lock-enabled") + .booleanType() + .defaultValue(false) + .withDescription("Enable the REST catalog's table commit lease capability."); + public static final ConfigOption COMMIT_TIMEOUT = key("commit.timeout") .durationType() @@ -3900,6 +3906,10 @@ public SequenceNumberInitMode writeSequenceNumberInitMode() { return options.get(WRITE_SEQUENCE_NUMBER_INIT_MODE); } + public boolean restCommitLockEnabled() { + return options.get(REST_COMMIT_LOCK_ENABLED); + } + public long commitTimeout() { return options.get(COMMIT_TIMEOUT) == null ? Long.MAX_VALUE diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java index 77a871d40007..64ad0a69a11d 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java @@ -45,6 +45,7 @@ import org.apache.paimon.rest.requests.AlterTableRequest; import org.apache.paimon.rest.requests.AlterViewRequest; import org.apache.paimon.rest.requests.AuthTableQueryRequest; +import org.apache.paimon.rest.requests.CommitLockRequest; import org.apache.paimon.rest.requests.CommitTableRequest; import org.apache.paimon.rest.requests.CreateBranchRequest; import org.apache.paimon.rest.requests.CreateDatabaseRequest; @@ -72,6 +73,7 @@ import org.apache.paimon.rest.requests.UpsertSemanticViewRequest; import org.apache.paimon.rest.responses.AlterDatabaseResponse; import org.apache.paimon.rest.responses.AuthTableQueryResponse; +import org.apache.paimon.rest.responses.CommitLockResponse; import org.apache.paimon.rest.responses.CommitTableResponse; import org.apache.paimon.rest.responses.ConfigResponse; import org.apache.paimon.rest.responses.CreatePartitionsResponse; @@ -734,6 +736,26 @@ public boolean commitSnapshot( return response.isSuccess(); } + public CommitLockResponse acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) { + return client.post( + resourcePaths.commitLock(identifier.getDatabaseName(), identifier.getObjectName()), + new CommitLockRequest(tableUuid, commitUser), + CommitLockResponse.class, + restAuthFunction); + } + + public boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) { + return client.post( + resourcePaths.commitLock( + identifier.getDatabaseName(), identifier.getObjectName()) + + "/renew", + new CommitLockRequest(tableUuid, commitUser), + CommitLockResponse.class, + restAuthFunction) + .isAcquired(); + } + /** * Rollback instant for table. * diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/ResourcePaths.java b/paimon-api/src/main/java/org/apache/paimon/rest/ResourcePaths.java index f3065708da9c..90ca5e9012cf 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/ResourcePaths.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/ResourcePaths.java @@ -196,6 +196,10 @@ public String commitTable(String databaseName, String objectName) { "commit"); } + public String commitLock(String databaseName, String objectName) { + return table(databaseName, objectName) + "/commit-lock"; + } + public String rollbackTable(String databaseName, String objectName) { return SLASH.join( V1, diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java b/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java new file mode 100644 index 000000000000..e0fc9b896609 --- /dev/null +++ b/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java @@ -0,0 +1,52 @@ +/* + * 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. + */ + +package org.apache.paimon.rest.requests; + +import org.apache.paimon.rest.RESTRequest; + +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonProperty; + +/** Request to acquire or renew a table commit lease owned by a writer. */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class CommitLockRequest implements RESTRequest { + + @JsonProperty("tableId") + private final String tableId; + + @JsonProperty("commitUser") + private final String commitUser; + + @JsonCreator + public CommitLockRequest( + @JsonProperty("tableId") String tableId, + @JsonProperty("commitUser") String commitUser) { + this.tableId = tableId; + this.commitUser = commitUser; + } + + public String getTableId() { + return tableId; + } + + public String getCommitUser() { + return commitUser; + } +} diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/responses/CommitLockResponse.java b/paimon-api/src/main/java/org/apache/paimon/rest/responses/CommitLockResponse.java new file mode 100644 index 000000000000..94c2f45810c1 --- /dev/null +++ b/paimon-api/src/main/java/org/apache/paimon/rest/responses/CommitLockResponse.java @@ -0,0 +1,90 @@ +/* + * 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. + */ + +package org.apache.paimon.rest.responses; + +import org.apache.paimon.Snapshot; +import org.apache.paimon.rest.RESTResponse; + +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonProperty; + +import javax.annotation.Nullable; + +/** Result of acquiring or renewing a commit lease; a null snapshot denotes an empty table. */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class CommitLockResponse implements RESTResponse { + + @JsonProperty("acquired") + private final boolean acquired; + + @JsonProperty("commitUser") + @Nullable + private final String commitUser; + + @JsonProperty("expiresAtMillis") + private final long expiresAtMillis; + + @JsonProperty("leaseMillis") + private final long leaseMillis; + + @JsonProperty("snapshot") + @Nullable + private final Snapshot snapshot; + + @JsonCreator + public CommitLockResponse( + @JsonProperty("acquired") boolean acquired, + @JsonProperty("commitUser") @Nullable String commitUser, + @JsonProperty("expiresAtMillis") long expiresAtMillis, + @JsonProperty("leaseMillis") long leaseMillis, + @JsonProperty("snapshot") @Nullable Snapshot snapshot) { + this.acquired = acquired; + this.commitUser = commitUser; + this.expiresAtMillis = expiresAtMillis; + this.leaseMillis = leaseMillis; + this.snapshot = snapshot; + } + + public static CommitLockResponse unavailable() { + return new CommitLockResponse(false, null, 0, 0, null); + } + + public boolean isAcquired() { + return acquired; + } + + @Nullable + public String getCommitUser() { + return commitUser; + } + + public long getExpiresAtMillis() { + return expiresAtMillis; + } + + public long getLeaseMillis() { + return leaseMillis; + } + + @Nullable + public Snapshot getSnapshot() { + return snapshot; + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java index 7e824845dbf9..999499c46623 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java @@ -747,6 +747,27 @@ boolean commitSnapshot( List statistics) throws Catalog.TableNotExistException; + /** + * Acquire a lease for this exact table UUID, branch and commit user. An empty result means + * another writer currently holds the lease. The grant includes the authoritative snapshot head. + * + *

Implementations must enforce ownership for all snapshot writers, including those that do + * not acquire leases, in the transaction that publishes the snapshot. A new successful + * publication releases the lease atomically; failed attempts stop renewal and expire. Existing + * snapshot compare-and-swap and conflict validation remain required. + */ + default Optional acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) + throws Catalog.TableNotExistException { + throw new UnsupportedOperationException("This catalog does not support commit leases."); + } + + /** Renew an active lease for its exact owner; return false after expiry or ownership loss. */ + default boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) + throws Catalog.TableNotExistException { + throw new UnsupportedOperationException("This catalog does not support commit leases."); + } + /** * Return the snapshot of table identified by the given {@link Identifier}. * diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java new file mode 100644 index 000000000000..7b87429fe379 --- /dev/null +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java @@ -0,0 +1,54 @@ +/* + * 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. + */ + +package org.apache.paimon.catalog; + +import org.apache.paimon.Snapshot; + +import javax.annotation.Nullable; + +/** A catalog lease and the authoritative snapshot observed when it was granted. */ +public class CatalogCommitLock { + + private final String commitUser; + private final long leaseMillis; + @Nullable private final Snapshot snapshot; + + public CatalogCommitLock(String commitUser, long leaseMillis, @Nullable Snapshot snapshot) { + if (commitUser == null || commitUser.isEmpty() || leaseMillis <= 0) { + throw new IllegalArgumentException( + "A commit lock requires a commitUser and a positive lease."); + } + this.commitUser = commitUser; + this.leaseMillis = leaseMillis; + this.snapshot = snapshot; + } + + public String commitUser() { + return commitUser; + } + + public long leaseMillis() { + return leaseMillis; + } + + @Nullable + public Snapshot snapshot() { + return snapshot; + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java index bd342e1013de..b8430f3161e2 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java @@ -328,6 +328,19 @@ public boolean commitSnapshot( identifier, tableUuid, baseSnapshotUuid, snapshot, statistics); } + @Override + public Optional acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) + throws TableNotExistException { + return wrapped.acquireCommitLock(identifier, tableUuid, commitUser); + } + + @Override + public boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) + throws TableNotExistException { + return wrapped.renewCommitLock(identifier, tableUuid, commitUser); + } + @Override public boolean supportsPartitionModification() { return wrapped.supportsPartitionModification(); diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java index 61ad72db0d8e..9d15b1336d7a 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java @@ -25,6 +25,7 @@ import org.apache.paimon.annotation.Experimental; import org.apache.paimon.annotation.VisibleForTesting; import org.apache.paimon.catalog.Catalog; +import org.apache.paimon.catalog.CatalogCommitLock; import org.apache.paimon.catalog.CatalogContext; import org.apache.paimon.catalog.CatalogUtils; import org.apache.paimon.catalog.Database; @@ -55,6 +56,7 @@ import org.apache.paimon.rest.exceptions.NotImplementedException; import org.apache.paimon.rest.exceptions.ServiceFailureException; import org.apache.paimon.rest.responses.AuthTableQueryResponse; +import org.apache.paimon.rest.responses.CommitLockResponse; import org.apache.paimon.rest.responses.ErrorResponse; import org.apache.paimon.rest.responses.GetDatabaseResponse; import org.apache.paimon.rest.responses.GetFunctionResponse; @@ -539,6 +541,40 @@ public boolean commitSnapshot( } } + @Override + public Optional acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) + throws TableNotExistException { + try { + CommitLockResponse response = api.acquireCommitLock(identifier, tableUuid, commitUser); + return response.isAcquired() + ? Optional.of( + new CatalogCommitLock( + response.getCommitUser(), + response.getLeaseMillis(), + response.getSnapshot())) + : Optional.empty(); + } catch (NoSuchResourceException e) { + throw new TableNotExistException(identifier, e); + } catch (ForbiddenException e) { + throw new TableNoPermissionException(identifier, e); + } catch (BadRequestException e) { + throw new IllegalArgumentException(e.getMessage(), e); + } + } + + @Override + public boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) + throws TableNotExistException { + try { + return api.renewCommitLock(identifier, tableUuid, commitUser); + } catch (NoSuchResourceException e) { + throw new TableNotExistException(identifier, e); + } catch (ForbiddenException e) { + throw new TableNoPermissionException(identifier, e); + } + } + @Override public void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) throws Catalog.TableNotExistException { diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java new file mode 100644 index 000000000000..c7d07fe83966 --- /dev/null +++ b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java @@ -0,0 +1,126 @@ +/* + * 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. + */ + +package org.apache.paimon.rest; + +import org.apache.paimon.Snapshot; +import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.options.Options; +import org.apache.paimon.rest.requests.CommitLockRequest; +import org.apache.paimon.rest.requests.CommitTableRequest; +import org.apache.paimon.rest.responses.CommitLockResponse; + +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.node.ObjectNode; + +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.concurrent.TimeUnit; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests the owner lease wire protocol without changing legacy snapshot requests. */ +class CommitLockProtocolTest { + @Test + void acquireRenewAndCommitUseTheSameOwnerWithoutToken() throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + Options options = new Options(); + options.set(RESTCatalogOptions.URI, server.url("/").toString()); + options.set(RESTCatalogOptions.TOKEN_PROVIDER, "bear"); + options.set(RESTCatalogOptions.TOKEN, "token"); + options.set(RESTCatalogInternalOptions.PREFIX, "catalog"); + RESTApi api = new RESTApi(options, false); + Identifier identifier = Identifier.create("db", "table"); + CommitLockResponse grant = + new CommitLockResponse(true, "morax-job", 100000, 60000, snapshot(1)); + server.enqueue(json(RESTApi.toJson(grant))); + CommitLockResponse acquired = + api.acquireCommitLock(identifier, "table-id", "morax-job"); + assertThat(acquired.isAcquired()).isTrue(); + assertThat(acquired.getSnapshot().id()).isEqualTo(1); + RecordedRequest acquire = server.takeRequest(10, TimeUnit.SECONDS); + assertThat(acquire.getPath()) + .isEqualTo("/v1/catalog/databases/db/tables/table/commit-lock"); + CommitLockRequest request = + RESTApi.fromJson(acquire.getBody().readUtf8(), CommitLockRequest.class); + assertThat(request.getTableId()).isEqualTo("table-id"); + assertThat(request.getCommitUser()).isEqualTo("morax-job"); + server.enqueue(json(RESTApi.toJson(grant))); + assertThat(api.renewCommitLock(identifier, "table-id", "morax-job")).isTrue(); + assertThat(server.takeRequest(10, TimeUnit.SECONDS).getPath()) + .endsWith("/commit-lock/renew"); + server.enqueue(json("{\"success\":true}")); + assertThat( + api.commitSnapshot( + identifier, + "table-id", + null, + snapshot(2), + Collections.emptyList())) + .isTrue(); + String commitBody = server.takeRequest(10, TimeUnit.SECONDS).getBody().readUtf8(); + ObjectNode payload = RESTApi.fromJson(commitBody, ObjectNode.class); + assertThat(payload.has("lockToken")).isFalse(); + assertThat(payload.has("commitUser")).isFalse(); + assertThat( + RESTApi.fromJson(commitBody, CommitTableRequest.class) + .getSnapshot() + .commitUser()) + .isEqualTo("morax-job"); + assertThat(RESTApi.fromJson(commitBody, CommitTableRequest.class).getTableId()) + .isEqualTo("table-id"); + } + } + + private static Snapshot snapshot(long id) { + return new Snapshot( + id, + 0L, + null, + null, + null, + null, + null, + null, + null, + "morax-job", + null, + 0L, + Snapshot.CommitKind.APPEND, + 1000L, + 0L, + 0L, + null, + null, + null, + null, + null, + null); + } + + private static MockResponse json(String body) { + return new MockResponse() + .setResponseCode(200) + .setBody(body) + .addHeader("Content-Type", "application/json"); + } +} From b99446207e3175d4ecebf012b39dbf26f142d122 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Sat, 10 Oct 2026 19:43:02 +0800 Subject: [PATCH 2/9] [rest] Reuse catalog lock factories for commit leases --- docs/docs/concepts/concurrency-control.md | 38 ++-- .../org/apache/paimon/catalog/Catalog.java | 21 -- .../paimon/catalog/CatalogCommitLock.java | 35 +-- .../apache/paimon/catalog/CatalogLock.java | 15 ++ .../apache/paimon/catalog/CatalogUtils.java | 4 +- .../paimon/catalog/DelegateCatalog.java | 13 -- .../org/apache/paimon/rest/RESTCatalog.java | 45 +--- .../apache/paimon/rest/RESTCatalogLock.java | 186 ++++++++++++++++ .../paimon/rest/RESTCatalogLockFactory.java | 39 ++++ .../org.apache.paimon.factories.Factory | 1 + .../paimon/rest/CommitLockProtocolTest.java | 85 ++++++++ .../paimon/rest/RESTCatalogLockTest.java | 205 ++++++++++++++++++ 12 files changed, 567 insertions(+), 120 deletions(-) create mode 100644 paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java create mode 100644 paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java create mode 100644 paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index 18c076a106f1..930c9d8a6f14 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -74,26 +74,28 @@ configuration. See [Catalog](./catalog) when choosing the metadata backend. ## REST Catalog Commit Leases -A REST catalog can expose optional table commit leases. Set `rest.commit.lock-enabled = true` -on the table to allow lease acquisition; the option defaults to `false`. Enabling this capability -does not automatically make writers acquire leases. The catalog API exposes acquisition and -renewal for callers that manage a commit lease explicitly. - -The client supplies the stable table UUID and its exact `commitUser` when acquiring and renewing. -A grant includes the server's current snapshot; the holder uses that head to rebuild and validate -its changes, renewing the lease while preparing and publishing. The existing snapshot commit -request is unchanged: the server checks ownership using `snapshot.commitUser` and the authenticated -caller. It must enforce this admission check for all writers in the same transaction as publication, -including writers that do not acquire leases. A new successful publication clears the lease -atomically. Abandoned attempts stop renewal and expire automatically; there is no explicit unlock API. +The REST catalog can coordinate snapshot publication using table commit leases. Set +`rest.commit.lock-enabled = true` on the table to allow lease acquisition; this capability option +is disabled by default and is independent of the catalog options `lock.enabled` and `lock.type`. +Enabling it does not automatically acquire leases for writers. + +`RESTCatalogLockFactory` uses the existing `CatalogLockFactory` mechanism to create a lease client. +Callers explicitly acquire a scope through `CatalogLock.acquireCommitLock`, supplying the stable +table UUID, branch and exact `commitUser`. The scope provides the authoritative snapshot head, +renews the lease while open, and detects expiry or renewal failure through `ensureValid`. +Closing a scope stops renewal; callers must check `ensureValid` before publishing. +The optional commit lease capability does not change existing `runWithLock` implementations. + +The snapshot commit request is unchanged. The server must check ownership using +`snapshot.commitUser` and the authenticated caller, and publish the snapshot in the same +transaction, including for writers that do not request leases. A new successful publication +clears the lease atomically. Abandoned attempts stop renewal and expire automatically; +there is no separate unlock RPC. This is a commit admission lease, not a generation or fencing token. Requests with the same -`commitUser` and authenticated caller share ownership. Use a unique commit user per logical writer -and preserve it during recovery. Snapshot UUID comparison and file conflict validation remain -required; leases cannot make stale file changes valid or guarantee success after lease loss. - -This REST table capability is independent of the catalog options `lock.enabled` and `lock.type`, -which configure the existing catalog lock used for filesystem publication. +`commitUser` and authenticated caller share ownership. Use a unique commit user per logical +writer and preserve it during recovery. Snapshot UUID comparison and file conflict validation +remain required; leases cannot make stale file changes valid or guarantee success after lease loss. ## Files conflict diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java index 999499c46623..7e824845dbf9 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java @@ -747,27 +747,6 @@ boolean commitSnapshot( List statistics) throws Catalog.TableNotExistException; - /** - * Acquire a lease for this exact table UUID, branch and commit user. An empty result means - * another writer currently holds the lease. The grant includes the authoritative snapshot head. - * - *

Implementations must enforce ownership for all snapshot writers, including those that do - * not acquire leases, in the transaction that publishes the snapshot. A new successful - * publication releases the lease atomically; failed attempts stop renewal and expire. Existing - * snapshot compare-and-swap and conflict validation remain required. - */ - default Optional acquireCommitLock( - Identifier identifier, String tableUuid, String commitUser) - throws Catalog.TableNotExistException { - throw new UnsupportedOperationException("This catalog does not support commit leases."); - } - - /** Renew an active lease for its exact owner; return false after expiry or ownership loss. */ - default boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) - throws Catalog.TableNotExistException { - throw new UnsupportedOperationException("This catalog does not support commit leases."); - } - /** * Return the snapshot of table identified by the given {@link Identifier}. * diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java index 7b87429fe379..a4a0a21249a7 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java @@ -22,33 +22,16 @@ import javax.annotation.Nullable; -/** A catalog lease and the authoritative snapshot observed when it was granted. */ -public class CatalogCommitLock { +/** A commit lease scope, including the authoritative snapshot observed when it was granted. */ +public interface CatalogCommitLock extends AutoCloseable { - private final String commitUser; - private final long leaseMillis; - @Nullable private final Snapshot snapshot; - - public CatalogCommitLock(String commitUser, long leaseMillis, @Nullable Snapshot snapshot) { - if (commitUser == null || commitUser.isEmpty() || leaseMillis <= 0) { - throw new IllegalArgumentException( - "A commit lock requires a commitUser and a positive lease."); - } - this.commitUser = commitUser; - this.leaseMillis = leaseMillis; - this.snapshot = snapshot; - } - - public String commitUser() { - return commitUser; - } + @Nullable + Snapshot snapshot(); - public long leaseMillis() { - return leaseMillis; - } + /** Fail before publication if the scope is closed or the lease is no longer usable. */ + void ensureValid(); - @Nullable - public Snapshot snapshot() { - return snapshot; - } + /** End the scope idempotently. Lease implementations stop renewal on close. */ + @Override + void close(); } diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java index 4f147c6aada7..1376a2d52531 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java @@ -21,6 +21,7 @@ import org.apache.paimon.annotation.Public; import java.io.Closeable; +import java.util.Optional; import java.util.concurrent.Callable; /** @@ -33,4 +34,18 @@ public interface CatalogLock extends Closeable { /** Run with catalog lock. The caller should tell catalog the database and table name. */ T runWithLock(String database, String table, Callable callable) throws Exception; + + /** + * Acquire a commit lease for the exact table UUID, branch and commit user. An empty result + * means another writer holds the lease. The scope covers head refresh, validation, preparation + * and publication; implementations must also gate writers that do not request leases. + * + *

This capability is separate from {@link #runWithLock}. Existing publication locks do not + * automatically support a larger commit scope. + */ + default Optional acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) throws Exception { + throw new UnsupportedOperationException( + "This catalog lock does not support commit leases."); + } } diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java index 98acf9fd8de3..24e667d73157 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java @@ -391,8 +391,8 @@ public static Table loadTable( tableIdentifier, metadata.uuid(), isRestCatalog && metadata.isExternal() ? null : catalog.catalogLoader(), - isRestCatalog ? null : lockFactory, - isRestCatalog ? null : lockContext, + isRestCatalog && metadata.isExternal() ? null : lockFactory, + isRestCatalog && metadata.isExternal() ? null : lockContext, catalogContext, catalog.supportsVersionManagement(), catalog.supportsPartitionModification()); diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java index b8430f3161e2..bd342e1013de 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java @@ -328,19 +328,6 @@ public boolean commitSnapshot( identifier, tableUuid, baseSnapshotUuid, snapshot, statistics); } - @Override - public Optional acquireCommitLock( - Identifier identifier, String tableUuid, String commitUser) - throws TableNotExistException { - return wrapped.acquireCommitLock(identifier, tableUuid, commitUser); - } - - @Override - public boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) - throws TableNotExistException { - return wrapped.renewCommitLock(identifier, tableUuid, commitUser); - } - @Override public boolean supportsPartitionModification() { return wrapped.supportsPartitionModification(); diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java index 9d15b1336d7a..95ee9afe6d89 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java @@ -25,8 +25,8 @@ import org.apache.paimon.annotation.Experimental; import org.apache.paimon.annotation.VisibleForTesting; import org.apache.paimon.catalog.Catalog; -import org.apache.paimon.catalog.CatalogCommitLock; import org.apache.paimon.catalog.CatalogContext; +import org.apache.paimon.catalog.CatalogLockContext; import org.apache.paimon.catalog.CatalogUtils; import org.apache.paimon.catalog.Database; import org.apache.paimon.catalog.Identifier; @@ -56,7 +56,6 @@ import org.apache.paimon.rest.exceptions.NotImplementedException; import org.apache.paimon.rest.exceptions.ServiceFailureException; import org.apache.paimon.rest.responses.AuthTableQueryResponse; -import org.apache.paimon.rest.responses.CommitLockResponse; import org.apache.paimon.rest.responses.ErrorResponse; import org.apache.paimon.rest.responses.GetDatabaseResponse; import org.apache.paimon.rest.responses.GetFunctionResponse; @@ -398,8 +397,8 @@ public Table getTable(Identifier identifier) throws TableNotExistException { path -> fileIOForData(path, identifier), this::fileIOFromOptions, this::loadTableMetadata, - null, - null, + new RESTCatalogLockFactory(), + CatalogLockContext.fromOptions(context.options()), context, true); } @@ -541,40 +540,6 @@ public boolean commitSnapshot( } } - @Override - public Optional acquireCommitLock( - Identifier identifier, String tableUuid, String commitUser) - throws TableNotExistException { - try { - CommitLockResponse response = api.acquireCommitLock(identifier, tableUuid, commitUser); - return response.isAcquired() - ? Optional.of( - new CatalogCommitLock( - response.getCommitUser(), - response.getLeaseMillis(), - response.getSnapshot())) - : Optional.empty(); - } catch (NoSuchResourceException e) { - throw new TableNotExistException(identifier, e); - } catch (ForbiddenException e) { - throw new TableNoPermissionException(identifier, e); - } catch (BadRequestException e) { - throw new IllegalArgumentException(e.getMessage(), e); - } - } - - @Override - public boolean renewCommitLock(Identifier identifier, String tableUuid, String commitUser) - throws TableNotExistException { - try { - return api.renewCommitLock(identifier, tableUuid, commitUser); - } catch (NoSuchResourceException e) { - throw new TableNotExistException(identifier, e); - } catch (ForbiddenException e) { - throw new TableNoPermissionException(identifier, e); - } - } - @Override public void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) throws Catalog.TableNotExistException { @@ -650,8 +615,8 @@ private Table toTable(String db, GetTableResponse response) { path -> fileIOForData(path, identifier), this::fileIOFromOptions, i -> toTableMetadata(db, response), - null, - null, + new RESTCatalogLockFactory(), + CatalogLockContext.fromOptions(context.options()), context, true); } catch (TableNotExistException e) { diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java new file mode 100644 index 000000000000..b09a5b9243e2 --- /dev/null +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java @@ -0,0 +1,186 @@ +/* + * 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. + */ + +package org.apache.paimon.rest; + +import org.apache.paimon.Snapshot; +import org.apache.paimon.catalog.Catalog; +import org.apache.paimon.catalog.CatalogCommitLock; +import org.apache.paimon.catalog.CatalogLock; +import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.rest.exceptions.BadRequestException; +import org.apache.paimon.rest.exceptions.ForbiddenException; +import org.apache.paimon.rest.exceptions.NoSuchResourceException; +import org.apache.paimon.rest.responses.CommitLockResponse; +import org.apache.paimon.utils.ExecutorThreadFactory; + +import javax.annotation.Nullable; + +import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.LongSupplier; + +/** + * REST commit leases. General catalog operations are not protected by the commit lease protocol. + */ +public class RESTCatalogLock implements CatalogLock { + + private static final ScheduledThreadPoolExecutor RENEWER = createRenewer(); + + private final RESTApi api; + private final ScheduledExecutorService renewer; + private final LongSupplier nanoTime; + + public RESTCatalogLock(RESTApi api) { + this(api, RENEWER); + } + + RESTCatalogLock(RESTApi api, ScheduledExecutorService renewer) { + this(api, renewer, System::nanoTime); + } + + RESTCatalogLock(RESTApi api, ScheduledExecutorService renewer, LongSupplier nanoTime) { + this.api = api; + this.renewer = renewer; + this.nanoTime = nanoTime; + } + + private static ScheduledThreadPoolExecutor createRenewer() { + ScheduledThreadPoolExecutor executor = + new ScheduledThreadPoolExecutor(2, new ExecutorThreadFactory("commit-lock-renew")); + executor.setRemoveOnCancelPolicy(true); + return executor; + } + + @Override + public T runWithLock(String database, String table, Callable callable) { + throw new UnsupportedOperationException( + "REST commit leases require a table UUID, branch and commit user."); + } + + @Override + public Optional acquireCommitLock( + Identifier identifier, String tableUuid, String commitUser) + throws Catalog.TableNotExistException { + if (tableUuid == null + || tableUuid.isEmpty() + || commitUser == null + || commitUser.isEmpty()) { + throw new IllegalArgumentException( + "A commit lease requires a table UUID and commit user."); + } + try { + long requestedAt = nanoTime.getAsLong(); + CommitLockResponse response = api.acquireCommitLock(identifier, tableUuid, commitUser); + if (!response.isAcquired()) { + return Optional.empty(); + } + if (!commitUser.equals(response.getCommitUser()) || response.getLeaseMillis() <= 0) { + throw new IllegalStateException("The server returned an invalid commit lease."); + } + return Optional.of(new Lease(identifier, tableUuid, commitUser, response, requestedAt)); + } catch (NoSuchResourceException e) { + throw new Catalog.TableNotExistException(identifier, e); + } catch (ForbiddenException e) { + throw new Catalog.TableNoPermissionException(identifier, e); + } catch (BadRequestException e) { + throw new IllegalArgumentException(e.getMessage(), e); + } + } + + private class Lease implements CatalogCommitLock { + private final Identifier identifier; + private final String tableUuid; + private final String commitUser; + @Nullable private final Snapshot snapshot; + private final ScheduledFuture renewal; + private final AtomicBoolean closed = new AtomicBoolean(); + private final long leaseNanos; + private volatile long renewedAt; + @Nullable private volatile Exception renewalFailure; + + private Lease( + Identifier identifier, + String tableUuid, + String commitUser, + CommitLockResponse response, + long requestedAt) { + this.identifier = identifier; + this.tableUuid = tableUuid; + this.commitUser = commitUser; + this.snapshot = response.getSnapshot(); + this.leaseNanos = TimeUnit.MILLISECONDS.toNanos(response.getLeaseMillis()); + this.renewedAt = requestedAt; + long interval = Math.max(1, response.getLeaseMillis() / 3); + this.renewal = + renewer.scheduleWithFixedDelay( + this::renew, interval, interval, TimeUnit.MILLISECONDS); + } + + private void renew() { + if (closed.get() || renewalFailure != null) { + return; + } + try { + ensureValid(); + long requestedAt = nanoTime.getAsLong(); + if (!api.renewCommitLock(identifier, tableUuid, commitUser)) { + renewalFailure = new IllegalStateException("The commit lease has expired."); + } else { + renewedAt = requestedAt; + } + } catch (Exception e) { + renewalFailure = e; + } + } + + @Override + public Snapshot snapshot() { + return snapshot; + } + + @Override + public void ensureValid() { + // Count response time conservatively and do not depend on the renewer being scheduled. + if (nanoTime.getAsLong() - renewedAt >= leaseNanos && renewalFailure == null) { + renewalFailure = new IllegalStateException("The commit lease has expired locally."); + } + if (closed.get() || renewalFailure != null) { + throw new IllegalStateException( + "The commit lease is no longer usable.", renewalFailure); + } + } + + @Override + public void close() { + if (closed.compareAndSet(false, true)) { + renewal.cancel(false); + } + } + } + + @Override + public void close() { + // RESTApi has no owned closeable resources; individual lease scopes stop renewal. + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java new file mode 100644 index 000000000000..f92cdd706821 --- /dev/null +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java @@ -0,0 +1,39 @@ +/* + * 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. + */ + +package org.apache.paimon.rest; + +import org.apache.paimon.catalog.CatalogLock; +import org.apache.paimon.catalog.CatalogLockContext; +import org.apache.paimon.catalog.CatalogLockFactory; + +/** Creates REST commit lease clients from serializable catalog options. */ +public class RESTCatalogLockFactory implements CatalogLockFactory { + + private static final long serialVersionUID = 1L; + + @Override + public String identifier() { + return RESTCatalogFactory.IDENTIFIER; + } + + @Override + public CatalogLock createLock(CatalogLockContext context) { + return new RESTCatalogLock(new RESTApi(context.options(), false)); + } +} diff --git a/paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory b/paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory index 3e678972d8e8..2e39575c5fe9 100644 --- a/paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory +++ b/paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory @@ -39,3 +39,4 @@ org.apache.paimon.mergetree.compact.aggregate.factory.FieldRoaringBitmap64AggFac org.apache.paimon.mergetree.compact.aggregate.factory.FieldSumAggFactory org.apache.paimon.mergetree.compact.aggregate.factory.FieldThetaSketchAggFactory org.apache.paimon.rest.RESTCatalogFactory +org.apache.paimon.rest.RESTCatalogLockFactory diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java index c7d07fe83966..a4bbb36cf958 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java @@ -19,11 +19,23 @@ package org.apache.paimon.rest; import org.apache.paimon.Snapshot; +import org.apache.paimon.catalog.CatalogCommitLock; +import org.apache.paimon.catalog.CatalogContext; +import org.apache.paimon.catalog.CatalogLock; +import org.apache.paimon.catalog.CatalogLockFactory; import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.catalog.RenamingSnapshotCommit; +import org.apache.paimon.factories.FactoryUtil; import org.apache.paimon.options.Options; import org.apache.paimon.rest.requests.CommitLockRequest; import org.apache.paimon.rest.requests.CommitTableRequest; import org.apache.paimon.rest.responses.CommitLockResponse; +import org.apache.paimon.rest.responses.GetTableResponse; +import org.apache.paimon.schema.Schema; +import org.apache.paimon.table.CatalogEnvironment; +import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.types.DataTypes; +import org.apache.paimon.utils.InstantiationUtil; import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.node.ObjectNode; @@ -31,7 +43,11 @@ import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import java.nio.file.Path; import java.util.Collections; import java.util.concurrent.TimeUnit; @@ -91,6 +107,75 @@ void acquireRenewAndCommitUseTheSameOwnerWithoutToken() throws Exception { } } + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void restTableCarriesSerializableCommitLockFactory(boolean external, @TempDir Path directory) + throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + Options options = new Options(); + options.set(RESTCatalogOptions.URI, server.url("/").toString()); + options.set(RESTCatalogOptions.TOKEN_PROVIDER, "bear"); + options.set(RESTCatalogOptions.TOKEN, "token"); + options.set(RESTCatalogInternalOptions.PREFIX, "catalog"); + options.set(RESTTokenFileIO.DATA_TOKEN_ENABLED, false); + GetTableResponse tableResponse = + new GetTableResponse( + "table-id", + "db", + "table$branch_dev", + directory.toUri().toString(), + external, + 0, + Schema.newBuilder().column("id", DataTypes.INT()).build(), + null, + 0, + null, + 0, + null); + server.enqueue(json(RESTApi.toJson(tableResponse))); + RESTCatalog catalog = new RESTCatalog(CatalogContext.create(options), false); + Identifier identifier = new Identifier("db", "table", "dev"); + FileStoreTable table = (FileStoreTable) catalog.getTable(identifier); + assertThat(server.takeRequest(10, TimeUnit.SECONDS).getPath()).contains("table"); + CatalogEnvironment environment = InstantiationUtil.clone(table.catalogEnvironment()); + assertThat(environment.uuid()).isEqualTo("table-id"); + if (external) { + assertThat(environment.catalogLoader()).isNull(); + assertThat(environment.lockFactory()).isNull(); + assertThat(environment.lockContext()).isNull(); + assertThat(environment.snapshotCommit(table.snapshotManager())) + .isInstanceOf(RenamingSnapshotCommit.class); + return; + } + assertThat(environment.lockFactory()).isInstanceOf(RESTCatalogLockFactory.class); + assertThat( + FactoryUtil.discoverFactory( + getClass().getClassLoader(), CatalogLockFactory.class, "rest")) + .isInstanceOf(RESTCatalogLockFactory.class); + server.enqueue( + json( + RESTApi.toJson( + new CommitLockResponse( + true, "morax-job", 100000, 60000, snapshot(1))))); + try (CatalogLock lock = + environment.lockFactory().createLock(environment.lockContext()); + CatalogCommitLock lease = + lock.acquireCommitLock(identifier, environment.uuid(), "morax-job") + .get()) { + assertThat(lease.snapshot().id()).isEqualTo(1); + lease.ensureValid(); + RecordedRequest acquire = server.takeRequest(10, TimeUnit.SECONDS); + assertThat(acquire.getPath()).endsWith("/commit-lock"); + assertThat(acquire.getHeader("Authorization")).isEqualTo("Bearer token"); + CommitLockRequest request = + RESTApi.fromJson(acquire.getBody().readUtf8(), CommitLockRequest.class); + assertThat(request.getTableId()).isEqualTo("table-id"); + assertThat(request.getCommitUser()).isEqualTo("morax-job"); + } + } + } + private static Snapshot snapshot(long id) { return new Snapshot( id, diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java new file mode 100644 index 000000000000..262f543b7874 --- /dev/null +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java @@ -0,0 +1,205 @@ +/* + * 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. + */ + +package org.apache.paimon.rest; + +import org.apache.paimon.Snapshot; +import org.apache.paimon.catalog.Catalog; +import org.apache.paimon.catalog.CatalogCommitLock; +import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.rest.exceptions.ForbiddenException; +import org.apache.paimon.rest.exceptions.NoSuchResourceException; +import org.apache.paimon.rest.responses.CommitLockResponse; + +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +/** Tests REST lease renewal and scope lifetime independently of snapshot publication. */ +class RESTCatalogLockTest { + private final RESTApi api = mock(RESTApi.class); + private final ScheduledExecutorService renewer = mock(ScheduledExecutorService.class); + private final ScheduledFuture future = mock(ScheduledFuture.class); + private final Identifier identifier = new Identifier("db", "table", "dev"); + private final AtomicLong clock = new AtomicLong(); + private final RESTCatalogLock lock = new RESTCatalogLock(api, renewer, clock::get); + + @Test + void grantedHeadIsAuthoritativeAndCloseIsIdempotent() throws Exception { + Snapshot head = mock(Snapshot.class); + grant("morax-job", 60000, head); + CatalogCommitLock lease = lock.acquireCommitLock(identifier, "table-id", "morax-job").get(); + assertThat(lease.snapshot()).isSameAs(head); + lease.ensureValid(); + lease.close(); + lease.close(); + verify(future).cancel(false); + assertThatThrownBy(lease::ensureValid).isInstanceOf(IllegalStateException.class); + } + + @Test + void renewalUsesExactOwnerAndStopsAfterClose() throws Exception { + grant("morax-job", 60000, null); + when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenReturn(true); + CatalogCommitLock lease = lock.acquireCommitLock(identifier, "table-id", "morax-job").get(); + Runnable renewal = renewal(); + renewal.run(); + lease.ensureValid(); + lease.close(); + renewal.run(); + verify(api, times(1)).renewCommitLock(identifier, "table-id", "morax-job"); + verify(future).cancel(false); + } + + @Test + void rejectedRenewalInvalidatesLease() throws Exception { + grant("morax-job", 60000, null); + when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenReturn(false); + try (CatalogCommitLock lease = + lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { + Runnable renewal = renewal(); + renewal.run(); + renewal.run(); + assertThatThrownBy(lease::ensureValid) + .isInstanceOf(IllegalStateException.class) + .hasCauseInstanceOf(IllegalStateException.class); + verify(api, times(1)).renewCommitLock(identifier, "table-id", "morax-job"); + } + } + + @Test + void renewalExceptionPreservesCause() throws Exception { + grant("morax-job", 60000, null); + RuntimeException failure = new RuntimeException("Disconnected"); + when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenThrow(failure); + try (CatalogCommitLock lease = + lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { + renewal().run(); + assertThatThrownBy(lease::ensureValid).hasCause(failure); + } + } + + @Test + void busyLeaseDoesNotStartRenewal() throws Exception { + when(api.acquireCommitLock(identifier, "table-id", "morax-job")) + .thenReturn(CommitLockResponse.unavailable()); + assertThat(lock.acquireCommitLock(identifier, "table-id", "morax-job")).isEmpty(); + verifyNoInteractions(renewer); + } + + @Test + void invalidGrantDoesNotStartRenewal() { + grant("other-job", 60000, null); + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + .isInstanceOf(IllegalStateException.class); + grant("morax-job", 0, null); + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + .isInstanceOf(IllegalStateException.class); + verifyNoInteractions(renewer); + } + + @Test + void missingIdentityAndGeneralLockAreRejectedBeforeRequest() { + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "", "morax-job")) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "")) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> lock.runWithLock("db", "table", () -> null)) + .isInstanceOf(UnsupportedOperationException.class); + verifyNoInteractions(api, renewer); + } + + @Test + void expiryIsDetectedEvenWhenRenewerHasNotRun() throws Exception { + grant("morax-job", 60000, null); + try (CatalogCommitLock lease = + lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(60000)); + assertThatThrownBy(lease::ensureValid).hasMessageContaining("no longer usable"); + renewal().run(); + verify(api, never()).renewCommitLock(identifier, "table-id", "morax-job"); + } + } + + @Test + void renewalExtendsLocalLifetimeAndIncludesResponseTime() throws Exception { + grant("morax-job", 60000, null); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenAnswer( + invocation -> { + clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10000)); + return true; + }); + try (CatalogCommitLock lease = + lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(20000)); + renewal().run(); + clock.set(TimeUnit.MILLISECONDS.toNanos(79999)); + lease.ensureValid(); + clock.set(TimeUnit.MILLISECONDS.toNanos(80000)); + assertThatThrownBy(lease::ensureValid).isInstanceOf(IllegalStateException.class); + } + } + + @Test + void acquirePreservesCatalogPermissionAndMissingTableErrors() { + ForbiddenException denied = new ForbiddenException("Denied"); + when(api.acquireCommitLock(identifier, "table-id", "morax-job")).thenThrow(denied); + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + .isInstanceOf(Catalog.TableNoPermissionException.class) + .hasCause(denied); + NoSuchResourceException missing = new NoSuchResourceException("table", "table", "Missing"); + doThrow(missing).when(api).acquireCommitLock(identifier, "table-id", "morax-job"); + assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + .isInstanceOf(Catalog.TableNotExistException.class) + .hasCause(missing); + verifyNoInteractions(renewer); + } + + private void grant(String owner, long leaseMillis, Snapshot head) { + when(api.acquireCommitLock(identifier, "table-id", "morax-job")) + .thenReturn(new CommitLockResponse(true, owner, 100000, leaseMillis, head)); + when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) + .thenAnswer(invocation -> future); + } + + private Runnable renewal() { + ArgumentCaptor callback = ArgumentCaptor.forClass(Runnable.class); + verify(renewer) + .scheduleWithFixedDelay( + callback.capture(), eq(20000L), eq(20000L), eq(TimeUnit.MILLISECONDS)); + return callback.getValue(); + } +} From db611442e97672b2e1f3c9089ec40d2c5894283a Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Sat, 10 Oct 2026 22:10:15 +0800 Subject: [PATCH 3/9] [rest] Fix commit lock request Jackson compatibility --- .../apache/paimon/rest/requests/CommitLockRequest.java | 3 +++ .../rest/requests/RequestJacksonCompatibilityTest.java | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java b/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java index e0fc9b896609..9a12f8272cd3 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/requests/CommitLockRequest.java @@ -24,6 +24,8 @@ import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonIgnoreProperties; import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonProperty; +import java.beans.ConstructorProperties; + /** Request to acquire or renew a table commit lease owned by a writer. */ @JsonIgnoreProperties(ignoreUnknown = true) public class CommitLockRequest implements RESTRequest { @@ -35,6 +37,7 @@ public class CommitLockRequest implements RESTRequest { private final String commitUser; @JsonCreator + @ConstructorProperties({"tableId", "commitUser"}) public CommitLockRequest( @JsonProperty("tableId") String tableId, @JsonProperty("commitUser") String commitUser) { diff --git a/paimon-api/src/test/java/org/apache/paimon/rest/requests/RequestJacksonCompatibilityTest.java b/paimon-api/src/test/java/org/apache/paimon/rest/requests/RequestJacksonCompatibilityTest.java index e7e8a5517e94..64dae0cb5ead 100644 --- a/paimon-api/src/test/java/org/apache/paimon/rest/requests/RequestJacksonCompatibilityTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/rest/requests/RequestJacksonCompatibilityTest.java @@ -72,6 +72,15 @@ public class RequestJacksonCompatibilityTest { "{\"select\":[\"id\",\"name\"]}", request -> assertThat(request.select()).containsExactly("id", "name"), "select"), + requestCase( + CommitLockRequest.class, + "{\"tableId\":\"table-id\",\"commitUser\":\"morax-job\"}", + request -> { + assertThat(request.getTableId()).isEqualTo("table-id"); + assertThat(request.getCommitUser()).isEqualTo("morax-job"); + }, + "tableId", + "commitUser"), requestCase( CreateBranchRequest.class, "{\"branch\":\"audit\",\"fromTag\":\"v1\"}", From 88388ad3dd4063c53ee6eb2adb823404fcc60761 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Sat, 10 Oct 2026 23:24:11 +0800 Subject: [PATCH 4/9] [rest] Expose commit locks through the catalog contract --- docs/docs/concepts/concurrency-control.md | 40 ++--- docs/static/rest-catalog-open-api.yaml | 9 + .../java/org/apache/paimon/rest/RESTApi.java | 12 +- .../rest/requests/RollbackTableRequest.java | 35 +++- .../org/apache/paimon/catalog/Catalog.java | 35 ++++ .../paimon/catalog/CatalogCommitLock.java | 37 ---- .../apache/paimon/catalog/CatalogLock.java | 38 ++-- .../paimon/catalog/DelegateCatalog.java | 22 +++ .../org/apache/paimon/operation/Lock.java | 95 ++++++++-- .../org/apache/paimon/rest/RESTCatalog.java | 41 ++++- .../apache/paimon/rest/RESTCatalogLock.java | 74 ++++++-- .../org/apache/paimon/operation/LockTest.java | 170 ++++++++++++++++++ .../paimon/rest/CommitLockProtocolTest.java | 63 +++++-- .../paimon/rest/RESTCatalogLockTest.java | 168 +++++++++-------- 14 files changed, 637 insertions(+), 202 deletions(-) delete mode 100644 paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java create mode 100644 paimon-core/src/test/java/org/apache/paimon/operation/LockTest.java diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index 930c9d8a6f14..3e546512c9ac 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -74,28 +74,28 @@ configuration. See [Catalog](./catalog) when choosing the metadata backend. ## REST Catalog Commit Leases -The REST catalog can coordinate snapshot publication using table commit leases. Set -`rest.commit.lock-enabled = true` on the table to allow lease acquisition; this capability option -is disabled by default and is independent of the catalog options `lock.enabled` and `lock.type`. -Enabling it does not automatically acquire leases for writers. - -`RESTCatalogLockFactory` uses the existing `CatalogLockFactory` mechanism to create a lease client. -Callers explicitly acquire a scope through `CatalogLock.acquireCommitLock`, supplying the stable -table UUID, branch and exact `commitUser`. The scope provides the authoritative snapshot head, -renews the lease while open, and detects expiry or renewal failure through `ensureValid`. -Closing a scope stops renewal; callers must check `ensureValid` before publishing. -The optional commit lease capability does not change existing `runWithLock` implementations. - -The snapshot commit request is unchanged. The server must check ownership using -`snapshot.commitUser` and the authenticated caller, and publish the snapshot in the same -transaction, including for writers that do not request leases. A new successful publication -clears the lease atomically. Abandoned attempts stop renewal and expire automatically; -there is no separate unlock RPC. +The REST catalog exposes table-bound commit leases through `Catalog.createLock` and the unified +`Lock.runWithLock` contract. Existing `CatalogLockFactory` implementations remain compatible +through the named `CatalogLock` adapter. Set `rest.commit.lock-enabled = true` on the table to +allow acquisition; it defaults to `false`. Callers supply their exact `commitUser` and table UUID. +Lock contention waits until `commit.timeout` when configured, independently of publication retries. +The client renews the lease during the protected operation and validates its monotonic deadline +before publishing. A failed operation stops renewal and its lease expires automatically. + +The server must validate the current table incarnation, branch, authenticated caller and exact +owner, and atomically publish the snapshot and clear the lease. All snapshot writers must honor +active leases, including writers that do not request one. Each successful snapshot publication +ends its lease; a later publication must acquire another scope. Snapshot comparison and file +conflict validation remain necessary. + +Rollback inside a lease supplies optional `tableId` and `commitUser` fields in addition to the +expected `fromSnapshot`. The server must validate the lease and expected head, roll back and clear +the lease atomically. Legacy rollback requests omit these fields and cannot change a leased head. This is a commit admission lease, not a generation or fencing token. Requests with the same -`commitUser` and authenticated caller share ownership. Use a unique commit user per logical -writer and preserve it during recovery. Snapshot UUID comparison and file conflict validation -remain required; leases cannot make stale file changes valid or guarantee success after lease loss. +commit user and authenticated caller share ownership. Use a unique commit user per logical writer +and preserve it during recovery. A server without the lease protocol rejects acquisition rather +than silently proceeding without protection. ## Files conflict diff --git a/docs/static/rest-catalog-open-api.yaml b/docs/static/rest-catalog-open-api.yaml index d3cd92cb6fd1..d94daa0428f3 100644 --- a/docs/static/rest-catalog-open-api.yaml +++ b/docs/static/rest-catalog-open-api.yaml @@ -3645,12 +3645,21 @@ components: $ref: '#/components/schemas/PartitionStatistics' RollbackTableRequest: type: object + description: >- + When rolling back inside a commit lease, supply tableId and commitUser together with + fromSnapshot. The server must validate table incarnation, current head, authenticated + caller and unexpired lease ownership, then roll back and clear the lease atomically. + A different owner or a request without ownership cannot change the head under a lease. properties: instant: $ref: '#/components/schemas/Instant' fromSnapshot: type: [ integer, "null" ] format: int64 + tableId: + type: [ string, "null" ] + commitUser: + type: [ string, "null" ] RollbackSchemaRequest: type: object required: diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java index 64ad0a69a11d..29dfcf77526c 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java @@ -783,7 +783,17 @@ public void rollbackTo(Identifier identifier, Instant instant) { * this table */ public void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) { - RollbackTableRequest request = new RollbackTableRequest(instant, fromSnapshot); + rollbackTo(identifier, null, instant, fromSnapshot, null); + } + + public void rollbackTo( + Identifier identifier, + @Nullable String tableUuid, + Instant instant, + @Nullable Long fromSnapshot, + @Nullable String commitUser) { + RollbackTableRequest request = + new RollbackTableRequest(instant, fromSnapshot, tableUuid, commitUser); client.post( resourcePaths.rollbackTable( identifier.getDatabaseName(), identifier.getObjectName()), diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java b/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java index 9c8ca25c66e9..b64ad92dd6d7 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java @@ -29,6 +29,8 @@ import javax.annotation.Nullable; +import java.beans.ConstructorProperties; + /** Request for rollback table. */ @JsonIgnoreProperties(ignoreUnknown = true) public class RollbackTableRequest implements RESTRequest { @@ -44,12 +46,43 @@ public class RollbackTableRequest implements RESTRequest { @Nullable private final Long fromSnapshot; + @JsonProperty("tableId") + @JsonInclude(JsonInclude.Include.NON_NULL) + @Nullable + private final String tableId; + + @JsonProperty("commitUser") + @JsonInclude(JsonInclude.Include.NON_NULL) + @Nullable + private final String commitUser; + + public RollbackTableRequest(Instant instant, @Nullable Long fromSnapshot) { + this(instant, fromSnapshot, null, null); + } + @JsonCreator + @ConstructorProperties({"instant", "fromSnapshot", "tableId", "commitUser"}) public RollbackTableRequest( @JsonProperty(FIELD_INSTANT) Instant instant, - @JsonProperty(FIELD_FROM_SNAPSHOT) @Nullable Long fromSnapshot) { + @JsonProperty(FIELD_FROM_SNAPSHOT) @Nullable Long fromSnapshot, + @JsonProperty("tableId") @Nullable String tableId, + @JsonProperty("commitUser") @Nullable String commitUser) { this.instant = instant; this.fromSnapshot = fromSnapshot; + this.tableId = tableId; + this.commitUser = commitUser; + } + + @JsonGetter("tableId") + @Nullable + public String getTableId() { + return tableId; + } + + @JsonGetter("commitUser") + @Nullable + public String getCommitUser() { + return commitUser; } @JsonGetter(FIELD_INSTANT) diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java index 7e824845dbf9..c638a9deb04a 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java @@ -25,6 +25,8 @@ import org.apache.paimon.consumer.ConsumerInfo; import org.apache.paimon.function.Function; import org.apache.paimon.function.FunctionChange; +import org.apache.paimon.operation.Lock; +import org.apache.paimon.options.Options; import org.apache.paimon.partition.Partition; import org.apache.paimon.partition.PartitionStatistics; import org.apache.paimon.predicate.Predicate; @@ -865,6 +867,17 @@ default void rollbackTo(Identifier identifier, Instant instant) void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) throws Catalog.TableNotExistException; + /** Roll back within a writer's table lock, preserving the table incarnation and owner. */ + default void rollbackTo( + Identifier identifier, + @Nullable String tableUuid, + Instant instant, + @Nullable Long fromSnapshot, + String commitUser) + throws Catalog.TableNotExistException { + rollbackTo(identifier, instant, fromSnapshot); + } + /** * Rollback table schema to a specific schema version. All schema versions greater than the * target will be deleted. This operation will fail if any snapshot, tag, or changelog @@ -1321,6 +1334,28 @@ void alterFunction( TableQueryAuthResult authTableQuery(Identifier identifier, @Nullable List select) throws TableNotExistException; + // ========================== Locking ============================== + + /** + * Create a lock bound to the table incarnation and exact writer identity. + * + *

The caller owns the returned lock and must close it. A lease may end when its snapshot is + * published; each additional snapshot must use a new {@link Lock#runWithLock} scope. Catalogs + * without configured locking return a no-op lock. + * + * @param identifier table and branch to lock + * @param tableUuid table incarnation, required by catalogs that fence table recreation + * @param commitUser exact logical writer identity, preserved across recovery + * @param tableOptions runtime table options controlling lock support + */ + default Lock createLock( + Identifier identifier, + @Nullable String tableUuid, + String commitUser, + Options tableOptions) { + return Lock.empty(); + } + // ==================== Catalog Information ========================== /** Catalog options for re-creating this catalog. */ diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java deleted file mode 100644 index a4a0a21249a7..000000000000 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogCommitLock.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * 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. - */ - -package org.apache.paimon.catalog; - -import org.apache.paimon.Snapshot; - -import javax.annotation.Nullable; - -/** A commit lease scope, including the authoritative snapshot observed when it was granted. */ -public interface CatalogCommitLock extends AutoCloseable { - - @Nullable - Snapshot snapshot(); - - /** Fail before publication if the scope is closed or the lease is no longer usable. */ - void ensureValid(); - - /** End the scope idempotently. Lease implementations stop renewal on close. */ - @Override - void close(); -} diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java index 1376a2d52531..9e3bdfdade84 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogLock.java @@ -19,33 +19,31 @@ package org.apache.paimon.catalog; import org.apache.paimon.annotation.Public; +import org.apache.paimon.operation.Lock; + +import javax.annotation.Nullable; import java.io.Closeable; -import java.util.Optional; import java.util.concurrent.Callable; -/** - * An interface that allows source and sink to use global lock to some transaction-related things. - * - * @since 0.4.0 - */ +/** Compatibility SPI for catalog lock factories. Runtime operations use {@link Lock}. */ @Public -public interface CatalogLock extends Closeable { +public interface CatalogLock extends Lock, Closeable { - /** Run with catalog lock. The caller should tell catalog the database and table name. */ T runWithLock(String database, String table, Callable callable) throws Exception; - /** - * Acquire a commit lease for the exact table UUID, branch and commit user. An empty result - * means another writer holds the lease. The scope covers head refresh, validation, preparation - * and publication; implementations must also gate writers that do not request leases. - * - *

This capability is separate from {@link #runWithLock}. Existing publication locks do not - * automatically support a larger commit scope. - */ - default Optional acquireCommitLock( - Identifier identifier, String tableUuid, String commitUser) throws Exception { - throw new UnsupportedOperationException( - "This catalog lock does not support commit leases."); + /** Bind table incarnation and writer identity when the backend requires them. */ + default T runWithLock( + Identifier identifier, + @Nullable String tableUuid, + @Nullable String commitUser, + Callable callable) + throws Exception { + return runWithLock(identifier.getDatabaseName(), identifier.getObjectName(), callable); + } + + @Override + default T runWithLock(Callable callable) throws Exception { + throw new UnsupportedOperationException("A catalog lock requires a table identifier."); } } diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java index bd342e1013de..c16d532a3d48 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java @@ -22,6 +22,8 @@ import org.apache.paimon.Snapshot; import org.apache.paimon.function.Function; import org.apache.paimon.function.FunctionChange; +import org.apache.paimon.operation.Lock; +import org.apache.paimon.options.Options; import org.apache.paimon.partition.Partition; import org.apache.paimon.partition.PartitionStatistics; import org.apache.paimon.predicate.Predicate; @@ -51,6 +53,15 @@ public DelegateCatalog(Catalog wrapped) { this.wrapped = wrapped; } + @Override + public Lock createLock( + Identifier identifier, + @Nullable String tableUuid, + String commitUser, + Options tableOptions) { + return wrapped.createLock(identifier, tableUuid, commitUser, tableOptions); + } + public Catalog wrapped() { return wrapped; } @@ -237,6 +248,17 @@ public PagedList listSchemasPaged( return wrapped.listSchemasPaged(identifier, maxResults, pageToken); } + @Override + public void rollbackTo( + Identifier identifier, + @Nullable String tableUuid, + Instant instant, + @Nullable Long fromSnapshot, + String commitUser) + throws Catalog.TableNotExistException { + wrapped.rollbackTo(identifier, tableUuid, instant, fromSnapshot, commitUser); + } + @Override public void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) throws Catalog.TableNotExistException { diff --git a/paimon-core/src/main/java/org/apache/paimon/operation/Lock.java b/paimon-core/src/main/java/org/apache/paimon/operation/Lock.java index b65e3c3d0c59..1f200bfc5b34 100644 --- a/paimon-core/src/main/java/org/apache/paimon/operation/Lock.java +++ b/paimon-core/src/main/java/org/apache/paimon/operation/Lock.java @@ -22,24 +22,24 @@ import org.apache.paimon.catalog.CatalogLock; import org.apache.paimon.catalog.Identifier; +import javax.annotation.Nullable; + import java.util.concurrent.Callable; -/** - * An interface that allows file store to use global lock to some transaction-related things. - * - * @since 0.4.0 - */ +/** A table lock covering an operation and its publication. */ @Public public interface Lock extends AutoCloseable { - /** Run with lock. */ T runWithLock(Callable callable) throws Exception; + /** Fail before publication when a held lease has expired or renewal has failed. */ + default void ensureValid() {} + static Lock empty() { return new EmptyLock(); } - /** An empty lock. */ + /** An operation with no external lock configured. */ class EmptyLock implements Lock { @Override public T runWithLock(Callable callable) throws Exception { @@ -51,32 +51,93 @@ public void close() {} } static Lock fromCatalog(CatalogLock lock, Identifier tablePath) { - if (lock == null) { - return new EmptyLock(); - } - return new CatalogLockImpl(lock, tablePath); + return fromCatalog(lock, tablePath, null, null); } - /** A {@link Lock} to wrap {@link CatalogLock}. */ - class CatalogLockImpl implements Lock { + static Lock fromCatalog( + CatalogLock lock, + Identifier tablePath, + @Nullable String tableUuid, + @Nullable String commitUser) { + return lock == null + ? empty() + : reentrant(new CatalogLockImpl(lock, tablePath, tableUuid, commitUser)); + } + /** Reuse the current scope for nested publication on the same thread. */ + static Lock reentrant(Lock lock) { + return lock instanceof ReentrantLock ? lock : new ReentrantLock(lock); + } + + /** Binds the legacy factory SPI to a table and writer. */ + class CatalogLockImpl implements Lock { private final CatalogLock catalogLock; private final Identifier tablePath; + @Nullable private final String tableUuid; + @Nullable private final String commitUser; - private CatalogLockImpl(CatalogLock catalogLock, Identifier tablePath) { + private CatalogLockImpl( + CatalogLock catalogLock, + Identifier tablePath, + @Nullable String tableUuid, + @Nullable String commitUser) { this.catalogLock = catalogLock; this.tablePath = tablePath; + this.tableUuid = tableUuid; + this.commitUser = commitUser; + } + + @Override + public T runWithLock(Callable callable) throws Exception { + return catalogLock.runWithLock(tablePath, tableUuid, commitUser, callable); + } + + @Override + public void ensureValid() { + catalogLock.ensureValid(); + } + + @Override + public void close() throws Exception { + catalogLock.close(); + } + } + + /** Keeps the outer operation's scope until preparation and publication both finish. */ + class ReentrantLock implements Lock { + private final Lock delegate; + private final ThreadLocal held = ThreadLocal.withInitial(() -> false); + + private ReentrantLock(Lock delegate) { + this.delegate = delegate; } @Override public T runWithLock(Callable callable) throws Exception { - return catalogLock.runWithLock( - tablePath.getDatabaseName(), tablePath.getObjectName(), callable); + if (held.get()) { + delegate.ensureValid(); + return callable.call(); + } + return delegate.runWithLock( + () -> { + held.set(true); + try { + delegate.ensureValid(); + return callable.call(); + } finally { + held.remove(); + } + }); + } + + @Override + public void ensureValid() { + delegate.ensureValid(); } @Override public void close() throws Exception { - this.catalogLock.close(); + delegate.close(); } } } diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java index 95ee9afe6d89..63b4f3114edb 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java @@ -26,7 +26,6 @@ import org.apache.paimon.annotation.VisibleForTesting; import org.apache.paimon.catalog.Catalog; import org.apache.paimon.catalog.CatalogContext; -import org.apache.paimon.catalog.CatalogLockContext; import org.apache.paimon.catalog.CatalogUtils; import org.apache.paimon.catalog.Database; import org.apache.paimon.catalog.Identifier; @@ -45,6 +44,7 @@ import org.apache.paimon.management.PermissionManagement; import org.apache.paimon.management.PolicyManagement; import org.apache.paimon.management.SemanticViewManagement; +import org.apache.paimon.operation.Lock; import org.apache.paimon.options.Options; import org.apache.paimon.partition.Partition; import org.apache.paimon.partition.PartitionStatistics; @@ -397,12 +397,30 @@ public Table getTable(Identifier identifier) throws TableNotExistException { path -> fileIOForData(path, identifier), this::fileIOFromOptions, this::loadTableMetadata, - new RESTCatalogLockFactory(), - CatalogLockContext.fromOptions(context.options()), + null, + null, context, true); } + @Override + public Lock createLock( + Identifier identifier, + @Nullable String tableUuid, + String commitUser, + Options tableOptions) { + if (!new CoreOptions(tableOptions).restCommitLockEnabled()) { + return Lock.empty(); + } + return Lock.fromCatalog( + new RESTCatalogLock( + new RESTApi(context.options(), false), + new CoreOptions(tableOptions).commitTimeout()), + identifier, + tableUuid, + commitUser); + } + @Override public Optional loadSnapshot(Identifier identifier) throws TableNotExistException { @@ -543,8 +561,19 @@ public boolean commitSnapshot( @Override public void rollbackTo(Identifier identifier, Instant instant, @Nullable Long fromSnapshot) throws Catalog.TableNotExistException { + rollbackTo(identifier, null, instant, fromSnapshot, null); + } + + @Override + public void rollbackTo( + Identifier identifier, + @Nullable String tableUuid, + Instant instant, + @Nullable Long fromSnapshot, + @Nullable String commitUser) + throws Catalog.TableNotExistException { try { - api.rollbackTo(identifier, instant, fromSnapshot); + api.rollbackTo(identifier, tableUuid, instant, fromSnapshot, commitUser); } catch (NoSuchResourceException e) { if (StringUtils.equals(e.resourceType(), ErrorResponse.RESOURCE_TYPE_SNAPSHOT)) { throw new IllegalArgumentException( @@ -615,8 +644,8 @@ private Table toTable(String db, GetTableResponse response) { path -> fileIOForData(path, identifier), this::fileIOFromOptions, i -> toTableMetadata(db, response), - new RESTCatalogLockFactory(), - CatalogLockContext.fromOptions(context.options()), + null, + null, context, true); } catch (TableNotExistException e) { diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java index b09a5b9243e2..b13089e67104 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java @@ -18,9 +18,7 @@ package org.apache.paimon.rest; -import org.apache.paimon.Snapshot; import org.apache.paimon.catalog.Catalog; -import org.apache.paimon.catalog.CatalogCommitLock; import org.apache.paimon.catalog.CatalogLock; import org.apache.paimon.catalog.Identifier; import org.apache.paimon.rest.exceptions.BadRequestException; @@ -31,7 +29,6 @@ import javax.annotation.Nullable; -import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; @@ -50,9 +47,15 @@ public class RESTCatalogLock implements CatalogLock { private final RESTApi api; private final ScheduledExecutorService renewer; private final LongSupplier nanoTime; + private final long acquireTimeoutMillis; + private final ThreadLocal currentLease = new ThreadLocal<>(); public RESTCatalogLock(RESTApi api) { - this(api, RENEWER); + this(api, Long.MAX_VALUE); + } + + public RESTCatalogLock(RESTApi api, long acquireTimeoutMillis) { + this(api, RENEWER, System::nanoTime, acquireTimeoutMillis); } RESTCatalogLock(RESTApi api, ScheduledExecutorService renewer) { @@ -60,9 +63,18 @@ public RESTCatalogLock(RESTApi api) { } RESTCatalogLock(RESTApi api, ScheduledExecutorService renewer, LongSupplier nanoTime) { + this(api, renewer, nanoTime, Long.MAX_VALUE); + } + + RESTCatalogLock( + RESTApi api, + ScheduledExecutorService renewer, + LongSupplier nanoTime, + long acquireTimeoutMillis) { this.api = api; this.renewer = renewer; this.nanoTime = nanoTime; + this.acquireTimeoutMillis = acquireTimeoutMillis; } private static ScheduledThreadPoolExecutor createRenewer() { @@ -79,9 +91,9 @@ public T runWithLock(String database, String table, Callable callable) { } @Override - public Optional acquireCommitLock( - Identifier identifier, String tableUuid, String commitUser) - throws Catalog.TableNotExistException { + public T runWithLock( + Identifier identifier, String tableUuid, String commitUser, Callable callable) + throws Exception { if (tableUuid == null || tableUuid.isEmpty() || commitUser == null @@ -89,16 +101,41 @@ public Optional acquireCommitLock( throw new IllegalArgumentException( "A commit lease requires a table UUID and commit user."); } + long startedAt = nanoTime.getAsLong(); + while (true) { + Lease lease = acquire(identifier, tableUuid, commitUser); + if (lease != null) { + currentLease.set(lease); + try { + lease.ensureValid(); + return callable.call(); + } finally { + currentLease.remove(); + lease.close(); + } + } + if (TimeUnit.NANOSECONDS.toMillis(nanoTime.getAsLong() - startedAt) + >= acquireTimeoutMillis) { + throw new IllegalStateException( + "Timed out acquiring commit lock for " + identifier); + } + Thread.sleep(Math.min(100, acquireTimeoutMillis)); + } + } + + @Nullable + private Lease acquire(Identifier identifier, String tableUuid, String commitUser) + throws Catalog.TableNotExistException { try { long requestedAt = nanoTime.getAsLong(); CommitLockResponse response = api.acquireCommitLock(identifier, tableUuid, commitUser); if (!response.isAcquired()) { - return Optional.empty(); + return null; } if (!commitUser.equals(response.getCommitUser()) || response.getLeaseMillis() <= 0) { throw new IllegalStateException("The server returned an invalid commit lease."); } - return Optional.of(new Lease(identifier, tableUuid, commitUser, response, requestedAt)); + return new Lease(identifier, tableUuid, commitUser, response, requestedAt); } catch (NoSuchResourceException e) { throw new Catalog.TableNotExistException(identifier, e); } catch (ForbiddenException e) { @@ -108,11 +145,19 @@ public Optional acquireCommitLock( } } - private class Lease implements CatalogCommitLock { + @Override + public void ensureValid() { + Lease lease = currentLease.get(); + if (lease == null) { + throw new IllegalStateException("No commit lease is held by this thread."); + } + lease.ensureValid(); + } + + private class Lease implements AutoCloseable { private final Identifier identifier; private final String tableUuid; private final String commitUser; - @Nullable private final Snapshot snapshot; private final ScheduledFuture renewal; private final AtomicBoolean closed = new AtomicBoolean(); private final long leaseNanos; @@ -128,7 +173,6 @@ private Lease( this.identifier = identifier; this.tableUuid = tableUuid; this.commitUser = commitUser; - this.snapshot = response.getSnapshot(); this.leaseNanos = TimeUnit.MILLISECONDS.toNanos(response.getLeaseMillis()); this.renewedAt = requestedAt; long interval = Math.max(1, response.getLeaseMillis() / 3); @@ -154,12 +198,6 @@ private void renew() { } } - @Override - public Snapshot snapshot() { - return snapshot; - } - - @Override public void ensureValid() { // Count response time conservatively and do not depend on the renewer being scheduled. if (nanoTime.getAsLong() - renewedAt >= leaseNanos && renewalFailure == null) { diff --git a/paimon-core/src/test/java/org/apache/paimon/operation/LockTest.java b/paimon-core/src/test/java/org/apache/paimon/operation/LockTest.java new file mode 100644 index 000000000000..5a37ee8fbe31 --- /dev/null +++ b/paimon-core/src/test/java/org/apache/paimon/operation/LockTest.java @@ -0,0 +1,170 @@ +/* + * 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. + */ + +package org.apache.paimon.operation; + +import org.apache.paimon.catalog.CatalogLock; +import org.apache.paimon.catalog.Identifier; + +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Tests the unified operation lock and compatibility with catalog lock factories. */ +class LockTest { + @Test + void nestedPublicationUsesOneScopeAndChecksValidity() throws Exception { + List events = new ArrayList<>(); + CatalogLock backend = + new CatalogLock() { + @Override + public T runWithLock(String database, String table, Callable action) + throws Exception { + assertThat(database).isEqualTo("db"); + assertThat(table).isEqualTo("table"); + events.add("acquire"); + try { + return action.call(); + } finally { + events.add("release"); + } + } + + @Override + public void ensureValid() { + events.add("validate"); + } + + @Override + public void close() { + events.add("close"); + } + }; + try (Lock lock = + Lock.fromCatalog(backend, Identifier.create("db", "table"), "table-id", "writer")) { + lock.runWithLock( + () -> { + events.add("head"); + return lock.runWithLock( + () -> { + events.add("publish"); + return true; + }); + }); + lock.runWithLock( + () -> { + events.add("next"); + return true; + }); + } + assertThat(events) + .isEqualTo( + Arrays.asList( + "acquire", + "validate", + "head", + "validate", + "publish", + "release", + "acquire", + "validate", + "next", + "release", + "close")); + } + + @Test + void expiredLeasePreventsNestedPublicationAndScopeIsReleased() throws Exception { + List events = new ArrayList<>(); + AtomicBoolean expired = new AtomicBoolean(); + Lock backend = + new Lock() { + @Override + public T runWithLock(Callable action) throws Exception { + events.add("acquire"); + try { + return action.call(); + } finally { + events.add("release"); + } + } + + @Override + public void ensureValid() { + if (expired.get()) { + throw new IllegalStateException("Expired"); + } + } + + @Override + public void close() {} + }; + Lock lock = Lock.reentrant(backend); + assertThatThrownBy( + () -> + lock.runWithLock( + () -> { + expired.set(true); + return lock.runWithLock( + () -> { + events.add("publish"); + return null; + }); + })) + .hasMessage("Expired"); + assertThat(events).containsExactly("acquire", "release"); + expired.set(false); + lock.runWithLock(() -> null); + assertThat(events).containsExactly("acquire", "release", "acquire", "release"); + } + + @Test + void runtimeIdentityReachesBackend() throws Exception { + Identifier identifier = new Identifier("db", "table", "dev"); + CatalogLock backend = + new CatalogLock() { + @Override + public T runWithLock(String database, String table, Callable action) { + throw new AssertionError("Runtime identity lost"); + } + + @Override + public T runWithLock( + Identifier actual, String uuid, String owner, Callable action) + throws Exception { + assertThat(actual).isEqualTo(identifier); + assertThat(uuid).isEqualTo("table-id"); + assertThat(owner).isEqualTo("writer"); + return action.call(); + } + + @Override + public void close() {} + }; + try (Lock lock = Lock.fromCatalog(backend, identifier, "table-id", "writer")) { + assertThat(lock.runWithLock(() -> "result")).isEqualTo("result"); + } + } +} diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java index a4bbb36cf958..2757f126e192 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java @@ -18,22 +18,25 @@ package org.apache.paimon.rest; +import org.apache.paimon.CoreOptions; import org.apache.paimon.Snapshot; -import org.apache.paimon.catalog.CatalogCommitLock; +import org.apache.paimon.catalog.Catalog; import org.apache.paimon.catalog.CatalogContext; -import org.apache.paimon.catalog.CatalogLock; import org.apache.paimon.catalog.CatalogLockFactory; import org.apache.paimon.catalog.Identifier; import org.apache.paimon.catalog.RenamingSnapshotCommit; import org.apache.paimon.factories.FactoryUtil; +import org.apache.paimon.operation.Lock; import org.apache.paimon.options.Options; import org.apache.paimon.rest.requests.CommitLockRequest; import org.apache.paimon.rest.requests.CommitTableRequest; +import org.apache.paimon.rest.requests.RollbackTableRequest; import org.apache.paimon.rest.responses.CommitLockResponse; import org.apache.paimon.rest.responses.GetTableResponse; import org.apache.paimon.schema.Schema; import org.apache.paimon.table.CatalogEnvironment; import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.table.Instant; import org.apache.paimon.types.DataTypes; import org.apache.paimon.utils.InstantiationUtil; @@ -107,9 +110,40 @@ void acquireRenewAndCommitUseTheSameOwnerWithoutToken() throws Exception { } } + @Test + void rollbackWithinLeaseCarriesOwnerAndLegacyPayloadStaysUnchanged() throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + Options options = new Options(); + options.set(RESTCatalogOptions.URI, server.url("/").toString()); + options.set(RESTCatalogOptions.TOKEN_PROVIDER, "bear"); + options.set(RESTCatalogOptions.TOKEN, "token"); + options.set(RESTCatalogInternalOptions.PREFIX, "catalog"); + RESTCatalog catalog = new RESTCatalog(CatalogContext.create(options), false); + Identifier identifier = new Identifier("db", "table", "dev"); + server.enqueue(json("{}")); + catalog.rollbackTo(identifier, "table-id", Instant.snapshot(1L), 2L, "morax-job"); + RecordedRequest owned = server.takeRequest(10, TimeUnit.SECONDS); + assertThat(owned.getPath()).endsWith("/rollback"); + RollbackTableRequest request = + RESTApi.fromJson(owned.getBody().readUtf8(), RollbackTableRequest.class); + assertThat(request.getTableId()).isEqualTo("table-id"); + assertThat(request.getCommitUser()).isEqualTo("morax-job"); + assertThat(request.getFromSnapshot()).isEqualTo(2L); + server.enqueue(json("{}")); + catalog.rollbackTo(identifier, Instant.snapshot(1L), 2L); + ObjectNode legacy = + RESTApi.fromJson( + server.takeRequest(10, TimeUnit.SECONDS).getBody().readUtf8(), + ObjectNode.class); + assertThat(legacy.has("tableId")).isFalse(); + assertThat(legacy.has("commitUser")).isFalse(); + } + } + @ParameterizedTest @ValueSource(booleans = {false, true}) - void restTableCarriesSerializableCommitLockFactory(boolean external, @TempDir Path directory) + void restTableCarriesSerializableCatalogLockEntry(boolean external, @TempDir Path directory) throws Exception { try (MockWebServer server = new MockWebServer()) { server.start(); @@ -134,6 +168,7 @@ void restTableCarriesSerializableCommitLockFactory(boolean external, @TempDir Pa 0, null); server.enqueue(json(RESTApi.toJson(tableResponse))); + options.set(RESTCatalogInternalOptions.PREFIX, "catalog"); RESTCatalog catalog = new RESTCatalog(CatalogContext.create(options), false); Identifier identifier = new Identifier("db", "table", "dev"); FileStoreTable table = (FileStoreTable) catalog.getTable(identifier); @@ -148,7 +183,7 @@ void restTableCarriesSerializableCommitLockFactory(boolean external, @TempDir Pa .isInstanceOf(RenamingSnapshotCommit.class); return; } - assertThat(environment.lockFactory()).isInstanceOf(RESTCatalogLockFactory.class); + assertThat(environment.lockFactory()).isNull(); assertThat( FactoryUtil.discoverFactory( getClass().getClassLoader(), CatalogLockFactory.class, "rest")) @@ -158,13 +193,19 @@ void restTableCarriesSerializableCommitLockFactory(boolean external, @TempDir Pa RESTApi.toJson( new CommitLockResponse( true, "morax-job", 100000, 60000, snapshot(1))))); - try (CatalogLock lock = - environment.lockFactory().createLock(environment.lockContext()); - CatalogCommitLock lease = - lock.acquireCommitLock(identifier, environment.uuid(), "morax-job") - .get()) { - assertThat(lease.snapshot().id()).isEqualTo(1); - lease.ensureValid(); + Options tableOptions = Options.fromMap(table.options()); + tableOptions.set(CoreOptions.REST_COMMIT_LOCK_ENABLED, true); + try (Catalog writerCatalog = environment.catalogLoader().load(); + Lock lock = + writerCatalog.createLock( + identifier, environment.uuid(), "morax-job", tableOptions)) { + assertThat( + lock.runWithLock( + () -> { + lock.ensureValid(); + return "published"; + })) + .isEqualTo("published"); RecordedRequest acquire = server.takeRequest(10, TimeUnit.SECONDS); assertThat(acquire.getPath()).endsWith("/commit-lock"); assertThat(acquire.getHeader("Authorization")).isEqualTo("Bearer token"); diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java index 262f543b7874..ed054663f9da 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java @@ -18,9 +18,7 @@ package org.apache.paimon.rest; -import org.apache.paimon.Snapshot; import org.apache.paimon.catalog.Catalog; -import org.apache.paimon.catalog.CatalogCommitLock; import org.apache.paimon.catalog.Identifier; import org.apache.paimon.rest.exceptions.ForbiddenException; import org.apache.paimon.rest.exceptions.NoSuchResourceException; @@ -29,6 +27,7 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import java.util.concurrent.Callable; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -42,99 +41,120 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; -/** Tests REST lease renewal and scope lifetime independently of snapshot publication. */ +/** Tests renewal, expiry and scoped execution of REST catalog locks. */ class RESTCatalogLockTest { private final RESTApi api = mock(RESTApi.class); private final ScheduledExecutorService renewer = mock(ScheduledExecutorService.class); private final ScheduledFuture future = mock(ScheduledFuture.class); private final Identifier identifier = new Identifier("db", "table", "dev"); private final AtomicLong clock = new AtomicLong(); - private final RESTCatalogLock lock = new RESTCatalogLock(api, renewer, clock::get); + private final RESTCatalogLock lock = new RESTCatalogLock(api, renewer, clock::get, 0); @Test - void grantedHeadIsAuthoritativeAndCloseIsIdempotent() throws Exception { - Snapshot head = mock(Snapshot.class); - grant("morax-job", 60000, head); - CatalogCommitLock lease = lock.acquireCommitLock(identifier, "table-id", "morax-job").get(); - assertThat(lease.snapshot()).isSameAs(head); - lease.ensureValid(); - lease.close(); - lease.close(); + void callbackIsProtectedAndLeaseEndsOnReturn() throws Exception { + grant("morax-job", 60000); + assertThat( + run( + () -> { + lock.ensureValid(); + return "published"; + })) + .isEqualTo("published"); + verify(future).cancel(false); + assertThatThrownBy(lock::ensureValid).hasMessageContaining("No commit lease"); + } + + @Test + void callbackFailureStopsRenewal() { + grant("morax-job", 60000); + RuntimeException failure = new RuntimeException("Preparation failed"); + assertThatThrownBy( + () -> + run( + () -> { + throw failure; + })) + .isSameAs(failure); verify(future).cancel(false); - assertThatThrownBy(lease::ensureValid).isInstanceOf(IllegalStateException.class); } @Test void renewalUsesExactOwnerAndStopsAfterClose() throws Exception { - grant("morax-job", 60000, null); + grant("morax-job", 60000); when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenReturn(true); - CatalogCommitLock lease = lock.acquireCommitLock(identifier, "table-id", "morax-job").get(); - Runnable renewal = renewal(); - renewal.run(); - lease.ensureValid(); - lease.close(); - renewal.run(); - verify(api, times(1)).renewCommitLock(identifier, "table-id", "morax-job"); + Runnable callback = + run( + () -> { + Runnable renewal = renewal(); + renewal.run(); + lock.ensureValid(); + return renewal; + }); + callback.run(); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); verify(future).cancel(false); } @Test void rejectedRenewalInvalidatesLease() throws Exception { - grant("morax-job", 60000, null); + grant("morax-job", 60000); when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenReturn(false); - try (CatalogCommitLock lease = - lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { - Runnable renewal = renewal(); - renewal.run(); - renewal.run(); - assertThatThrownBy(lease::ensureValid) - .isInstanceOf(IllegalStateException.class) - .hasCauseInstanceOf(IllegalStateException.class); - verify(api, times(1)).renewCommitLock(identifier, "table-id", "morax-job"); - } + run( + () -> { + renewal().run(); + renewal().run(); + assertThatThrownBy(lock::ensureValid) + .hasCauseInstanceOf(IllegalStateException.class); + return null; + }); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); } @Test void renewalExceptionPreservesCause() throws Exception { - grant("morax-job", 60000, null); + grant("morax-job", 60000); RuntimeException failure = new RuntimeException("Disconnected"); when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenThrow(failure); - try (CatalogCommitLock lease = - lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { - renewal().run(); - assertThatThrownBy(lease::ensureValid).hasCause(failure); - } + run( + () -> { + renewal().run(); + assertThatThrownBy(lock::ensureValid).hasCause(failure); + return null; + }); } @Test - void busyLeaseDoesNotStartRenewal() throws Exception { + void busyTimeoutDoesNotExecuteCallbackOrStartRenewal() { when(api.acquireCommitLock(identifier, "table-id", "morax-job")) .thenReturn(CommitLockResponse.unavailable()); - assertThat(lock.acquireCommitLock(identifier, "table-id", "morax-job")).isEmpty(); + assertThatThrownBy( + () -> + run( + () -> { + throw new AssertionError(); + })) + .hasMessageContaining("Timed out acquiring"); verifyNoInteractions(renewer); } @Test void invalidGrantDoesNotStartRenewal() { - grant("other-job", 60000, null); - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) - .isInstanceOf(IllegalStateException.class); - grant("morax-job", 0, null); - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) - .isInstanceOf(IllegalStateException.class); + grant("other-job", 60000); + assertThatThrownBy(() -> run(() -> null)).isInstanceOf(IllegalStateException.class); + grant("morax-job", 0); + assertThatThrownBy(() -> run(() -> null)).isInstanceOf(IllegalStateException.class); verifyNoInteractions(renewer); } @Test void missingIdentityAndGeneralLockAreRejectedBeforeRequest() { - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "", "morax-job")) + assertThatThrownBy(() -> lock.runWithLock(identifier, "", "morax-job", () -> null)) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "")) + assertThatThrownBy(() -> lock.runWithLock(identifier, "table-id", "", () -> null)) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> lock.runWithLock("db", "table", () -> null)) .isInstanceOf(UnsupportedOperationException.class); @@ -143,54 +163,60 @@ void missingIdentityAndGeneralLockAreRejectedBeforeRequest() { @Test void expiryIsDetectedEvenWhenRenewerHasNotRun() throws Exception { - grant("morax-job", 60000, null); - try (CatalogCommitLock lease = - lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { - clock.set(TimeUnit.MILLISECONDS.toNanos(60000)); - assertThatThrownBy(lease::ensureValid).hasMessageContaining("no longer usable"); - renewal().run(); - verify(api, never()).renewCommitLock(identifier, "table-id", "morax-job"); - } + grant("morax-job", 60000); + run( + () -> { + clock.set(TimeUnit.MILLISECONDS.toNanos(60000)); + assertThatThrownBy(lock::ensureValid).hasMessageContaining("no longer usable"); + renewal().run(); + verify(api, never()).renewCommitLock(identifier, "table-id", "morax-job"); + return null; + }); } @Test void renewalExtendsLocalLifetimeAndIncludesResponseTime() throws Exception { - grant("morax-job", 60000, null); + grant("morax-job", 60000); when(api.renewCommitLock(identifier, "table-id", "morax-job")) .thenAnswer( invocation -> { clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(10000)); return true; }); - try (CatalogCommitLock lease = - lock.acquireCommitLock(identifier, "table-id", "morax-job").get()) { - clock.set(TimeUnit.MILLISECONDS.toNanos(20000)); - renewal().run(); - clock.set(TimeUnit.MILLISECONDS.toNanos(79999)); - lease.ensureValid(); - clock.set(TimeUnit.MILLISECONDS.toNanos(80000)); - assertThatThrownBy(lease::ensureValid).isInstanceOf(IllegalStateException.class); - } + run( + () -> { + clock.set(TimeUnit.MILLISECONDS.toNanos(20000)); + renewal().run(); + clock.set(TimeUnit.MILLISECONDS.toNanos(79999)); + lock.ensureValid(); + clock.set(TimeUnit.MILLISECONDS.toNanos(80000)); + assertThatThrownBy(lock::ensureValid).isInstanceOf(IllegalStateException.class); + return null; + }); } @Test void acquirePreservesCatalogPermissionAndMissingTableErrors() { ForbiddenException denied = new ForbiddenException("Denied"); when(api.acquireCommitLock(identifier, "table-id", "morax-job")).thenThrow(denied); - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + assertThatThrownBy(() -> run(() -> null)) .isInstanceOf(Catalog.TableNoPermissionException.class) .hasCause(denied); NoSuchResourceException missing = new NoSuchResourceException("table", "table", "Missing"); doThrow(missing).when(api).acquireCommitLock(identifier, "table-id", "morax-job"); - assertThatThrownBy(() -> lock.acquireCommitLock(identifier, "table-id", "morax-job")) + assertThatThrownBy(() -> run(() -> null)) .isInstanceOf(Catalog.TableNotExistException.class) .hasCause(missing); verifyNoInteractions(renewer); } - private void grant(String owner, long leaseMillis, Snapshot head) { + private T run(Callable callable) throws Exception { + return lock.runWithLock(identifier, "table-id", "morax-job", callable); + } + + private void grant(String owner, long leaseMillis) { when(api.acquireCommitLock(identifier, "table-id", "morax-job")) - .thenReturn(new CommitLockResponse(true, owner, 100000, leaseMillis, head)); + .thenReturn(new CommitLockResponse(true, owner, 100000, leaseMillis, null)); when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) .thenAnswer(invocation -> future); } From b4d10b3bd41876b6e0d8f8256c659354fd0048e6 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Tue, 13 Oct 2026 11:03:39 +0800 Subject: [PATCH 5/9] [rest] Separate table lease capability from writer acquisition --- docs/docs/concepts/concurrency-control.md | 14 +++++- docs/generated/core_configuration.html | 8 ++- docs/static/rest-catalog-open-api.yaml | 4 +- .../java/org/apache/paimon/CoreOptions.java | 17 ++++++- .../org/apache/paimon/AbstractFileStore.java | 2 +- .../paimon/rest/RESTCatalogLockFactory.java | 7 ++- .../paimon/rest/CommitLockProtocolTest.java | 1 + .../rest/RESTCommitLockIntegrationTest.java | 50 +++++++++++++++++-- 8 files changed, 92 insertions(+), 11 deletions(-) diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index 3347789f579b..71bfbc187c59 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -105,8 +105,12 @@ provide a storage fencing token. Catalog-managed Paimon tables in the REST catalog expose table-bound commit leases through `Catalog.createLock` and the unified -`Lock.acquire` / `Lock.runWithLock` contract. Set `rest.commit.lock-enabled = true` on the table to -allow acquisition; it defaults to `false`. External tables keep filesystem publication. +`Lock.acquire` / `Lock.runWithLock` contract. Set the persisted table property +`rest.commit.lock-enabled = true` to enable server-side lease enforcement. Independently, set +`rest.commit.acquire-lock = true` in a writer's dynamic options to acquire a lease for each +publication attempt. Both options default to `false`. Enabling the table capability alone does +not make clients acquire leases. Requesting acquisition without the table capability fails. +External tables keep filesystem publication. Callers supply their exact `commitUser` and table UUID. Leases require the target branch's catalog-resolved UUID. Reload a view switched to another branch from the catalog before using leases. @@ -122,6 +126,12 @@ active leases, including writers that do not request one. Each successful snapsh ends its lease; a later publication must acquire another scope. Snapshot comparison and file conflict validation remain necessary. +For a table with lease enforcement disabled, the server can skip lease storage access entirely. +For a table with enforcement enabled, even writers with `rest.commit.acquire-lock = false` must +respect another writer's active lease. The server uses its own persisted table property for this +decision, rather than client options. Capability changes must be serialized with publication and +lease acquisition; disabling enforcement must not bypass an active lease. + Rollback inside a lease supplies `tableId` and `commitUser` in addition to the expected `fromSnapshot`. The server must validate the lease and expected head, roll back and clear the lease atomically. Ordinary rollback still supplies a known table UUID but omits `commitUser`; it cannot diff --git a/docs/generated/core_configuration.html b/docs/generated/core_configuration.html index 36ad11bfbb44..4c30f37c5a87 100644 --- a/docs/generated/core_configuration.html +++ b/docs/generated/core_configuration.html @@ -1470,11 +1470,17 @@ String Time field for record level expire. It supports the following types: `timestamps in seconds with INT`,`timestamps in seconds with BIGINT`, `timestamps in milliseconds with BIGINT` or `timestamp`. + +

rest.commit.acquire-lock
+ false + Boolean + Whether this writer acquires a REST commit lease for each publication attempt. Can be set dynamically for a writer and requires rest.commit.lock-enabled on the server-side table. +
rest.commit.lock-enabled
false Boolean - Enable the REST catalog's table commit lease capability. + Enable the REST catalog's server-side table commit lease capability. Writers acquire a lease only when rest.commit.acquire-lock is enabled.
row-tracking.enabled
diff --git a/docs/static/rest-catalog-open-api.yaml b/docs/static/rest-catalog-open-api.yaml index 0c5ae49c621d..fb62fd483149 100644 --- a/docs/static/rest-catalog-open-api.yaml +++ b/docs/static/rest-catalog-open-api.yaml @@ -644,7 +644,9 @@ paths: schema: type: string description: >- - Requires rest.commit.lock-enabled=true and the same permission as committing snapshots. + Requires the server-side table property rest.commit.lock-enabled=true and the same + permission as committing snapshots. Clients opt into acquisition separately using + rest.commit.acquire-lock=true; that client option does not enable the table capability. Ownership is the exact commitUser together with the authenticated caller. Contention returns HTTP 200 with acquired=false. A grant includes the authoritative snapshot head. Renewal never revives an expired lease. Publication retains snapshot CAS validation diff --git a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java index 1b6080148e57..d44370d760fc 100644 --- a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java +++ b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java @@ -977,7 +977,18 @@ public InlineElement getDescription() { key("rest.commit.lock-enabled") .booleanType() .defaultValue(false) - .withDescription("Enable the REST catalog's table commit lease capability."); + .withDescription( + "Enable the REST catalog's server-side table commit lease capability. " + + "Writers acquire a lease only when rest.commit.acquire-lock is enabled."); + + public static final ConfigOption REST_COMMIT_ACQUIRE_LOCK = + key("rest.commit.acquire-lock") + .booleanType() + .defaultValue(false) + .withDescription( + "Whether this writer acquires a REST commit lease for each publication " + + "attempt. Can be set dynamically for a writer and requires " + + "rest.commit.lock-enabled on the server-side table."); public static final ConfigOption COMMIT_TIMEOUT = key("commit.timeout") @@ -3910,6 +3921,10 @@ public boolean restCommitLockEnabled() { return options.get(REST_COMMIT_LOCK_ENABLED); } + public boolean restCommitAcquireLock() { + return options.get(REST_COMMIT_ACQUIRE_LOCK); + } + public long commitTimeout() { return options.get(COMMIT_TIMEOUT) == null ? Long.MAX_VALUE diff --git a/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java b/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java index 37445eefcde0..fbd43c960ff5 100644 --- a/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java +++ b/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java @@ -308,7 +308,7 @@ public FileStoreCommitImpl newCommit(String commitUser, FileStoreTable table) { scanner); CommitRollback rollback = null; TableRollback tableRollback = - options.restCommitLockEnabled() + options.restCommitAcquireLock() ? catalogEnvironment.catalogTableRollback(options.branch(), commitUser) : catalogEnvironment.catalogTableRollback(null, null); if (tableRollback != null) { diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java index f67617c55b54..bb91fd016b3d 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLockFactory.java @@ -27,6 +27,8 @@ import javax.annotation.Nullable; +import static org.apache.paimon.utils.Preconditions.checkArgument; + /** Creates REST commit lease clients from serializable catalog options. */ public class RESTCatalogLockFactory implements CatalogLockFactory { @@ -45,9 +47,12 @@ public Lock createLock( @Nullable String commitUser, Options tableOptions) { CoreOptions options = new CoreOptions(tableOptions); - if (!options.restCommitLockEnabled()) { + if (!options.restCommitAcquireLock()) { return Lock.empty(); } + checkArgument( + options.restCommitLockEnabled(), + "rest.commit.acquire-lock requires rest.commit.lock-enabled on the table."); return new RESTCatalogLock( new RESTApi(context.options(), false), identifier, diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java index f78ab8a9dc7b..b4aee172efd7 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java @@ -206,6 +206,7 @@ void restTableCarriesSerializableCatalogLockEntry(boolean external, @TempDir Pat true, "morax-job", 100000, 60000, snapshot(1))))); Options tableOptions = Options.fromMap(table.options()); tableOptions.set(CoreOptions.REST_COMMIT_LOCK_ENABLED, true); + tableOptions.set(CoreOptions.REST_COMMIT_ACQUIRE_LOCK, true); try (Catalog writerCatalog = environment.catalogLoader().load(); Lock lock = writerCatalog.createLock( diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java index 3add8665abff..56f1a8624828 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java @@ -81,6 +81,7 @@ import static org.apache.paimon.data.BinaryRow.EMPTY_ROW; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** Exercises REST leases through the real file-store commit path. */ class RESTCommitLockIntegrationTest { @@ -99,12 +100,52 @@ void disabledFactoryDoesNotRequireRestConfigurationOrWriterIdentity() { } @ParameterizedTest - @CsvSource({"false,false", "false,true", "true,false", "true,true"}) + @ValueSource(booleans = {false, true}) + void tableCapabilityDoesNotMakeAClientAcquireByDefault(boolean capability) { + Options options = new Options(); + options.set(CoreOptions.REST_COMMIT_LOCK_ENABLED, capability); + Lock lock = + new RESTCatalogLockFactory() + .createLock( + CatalogLockContext.fromOptions(new Options()), + null, + null, + null, + options); + assertThat(lock).isInstanceOf(Lock.EmptyLock.class); + } + + @Test + void acquisitionRequiresTheTableCapabilityBeforeRestRequests() { + Options options = new Options(); + options.set("rest.commit.acquire-lock", "true"); + assertThatThrownBy( + () -> + new RESTCatalogLockFactory() + .createLock( + CatalogLockContext.fromOptions(new Options()), + null, + null, + null, + options)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("rest.commit.lock-enabled"); + } + + @ParameterizedTest + @CsvSource({ + "false,false,false", "false,false,true", "true,false,false", + "true,false,true", "true,true,false", "true,true,true" + }) void commitsAcquireBeforeHeadAndReacquireAfterConflict( - boolean locking, boolean firstPublicationConflicts, @TempDir Path directory) + boolean capability, + boolean locking, + boolean firstPublicationConflicts, + @TempDir Path directory) throws Exception { Options tableOptions = new Options(); - tableOptions.set(CoreOptions.REST_COMMIT_LOCK_ENABLED, locking); + tableOptions.set(CoreOptions.REST_COMMIT_LOCK_ENABLED, capability); + tableOptions.set(CoreOptions.REST_COMMIT_ACQUIRE_LOCK, locking); tableOptions.set(CoreOptions.COMMIT_MAX_RETRIES, 2); tableOptions.set(CoreOptions.COMMIT_MIN_RETRY_WAIT, Duration.ofMillis(1)); tableOptions.set(CoreOptions.COMMIT_MAX_RETRY_WAIT, Duration.ofMillis(1)); @@ -226,7 +267,8 @@ void compactionConflictRollsBackWithWriterContextAndReacquires( Schema schema = Schema.newBuilder() .column("id", DataTypes.INT()) - .option(CoreOptions.REST_COMMIT_LOCK_ENABLED.key(), String.valueOf(locking)) + .option(CoreOptions.REST_COMMIT_LOCK_ENABLED.key(), "true") + .option(CoreOptions.REST_COMMIT_ACQUIRE_LOCK.key(), String.valueOf(locking)) .option(CoreOptions.COMMIT_MIN_RETRY_WAIT.key(), "1 ms") .option(CoreOptions.COMMIT_MAX_RETRY_WAIT.key(), "1 ms") .build(); From f05df62bcf751619c6605e419c2037c850988a56 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Tue, 13 Oct 2026 12:27:17 +0800 Subject: [PATCH 6/9] [rest] Share executors for commit lease renewal --- docs/docs/concepts/concurrency-control.md | 10 +- .../apache/paimon/rest/RESTCatalogLock.java | 77 +++- .../paimon/rest/RESTCatalogLockTest.java | 343 ++++++++++++++++-- 3 files changed, 374 insertions(+), 56 deletions(-) diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index 71bfbc187c59..a925a7a41f96 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -115,10 +115,12 @@ Callers supply their exact `commitUser` and table UUID. Leases require the target branch's catalog-resolved UUID. Reload a view switched to another branch from the catalog before using leases. Lock contention waits until `commit.timeout` when configured, independently of publication retries. -Each scope owns a renewal worker, which is stopped when the scope ends. The client validates -the lease's monotonic deadline before publishing. A failed operation stops renewal and its lease -expires automatically. The server keeps the lease duration fixed for each scope; duration changes -take effect on the next acquisition. +Scopes share a timer and a cached worker pool. The timer dispatches RPCs to workers so a blocked +renewal cannot block other scopes' scheduling. Each scope permits only one in-flight renewal and +cancels its own scheduled and in-flight renewal tasks when it ends, without stopping the shared +executors. The client validates the lease's monotonic deadline before publishing. A failed operation +stops renewal and its lease expires automatically. The server keeps the lease duration fixed for each +scope; duration changes take effect on the next acquisition. The server must validate the current table incarnation, branch, authenticated caller and exact owner, and atomically publish the snapshot and clear the lease. All snapshot writers must honor diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java index 820544288d89..1f902f3f35fb 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java @@ -29,13 +29,15 @@ import javax.annotation.Nullable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.FutureTask; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.LongSupplier; -import java.util.function.Supplier; import static org.apache.paimon.utils.Preconditions.checkArgument; import static org.apache.paimon.utils.Preconditions.checkNotNull; @@ -43,11 +45,16 @@ /** A renewable REST commit lease bound to a table, branch and writer. */ public class RESTCatalogLock implements Lock { + private static final ScheduledExecutorService RENEWAL_SCHEDULER = createRenewalScheduler(); + private static final ExecutorService RENEWAL_WORKERS = + Executors.newCachedThreadPool(new ExecutorThreadFactory("rest-commit-lock-renew")); + private final RESTApi api; private final Identifier identifier; private final String tableUuid; private final String commitUser; - private final Supplier renewerFactory; + private final ScheduledExecutorService scheduler; + private final ExecutorService workers; private final LongSupplier nanoTime; private final long acquireTimeoutMillis; private final ThreadLocal currentLease = new ThreadLocal<>(); @@ -64,7 +71,8 @@ public RESTCatalogLock( tableUuid, commitUser, acquireTimeoutMillis, - RESTCatalogLock::createRenewer, + RENEWAL_SCHEDULER, + RENEWAL_WORKERS, System::nanoTime); } @@ -74,7 +82,8 @@ public RESTCatalogLock( String tableUuid, String commitUser, long acquireTimeoutMillis, - Supplier renewerFactory, + ScheduledExecutorService scheduler, + ExecutorService workers, LongSupplier nanoTime) { checkArgument( tableUuid != null && !tableUuid.isEmpty(), "A commit lease requires a table UUID."); @@ -88,14 +97,15 @@ public RESTCatalogLock( this.tableUuid = tableUuid; this.commitUser = commitUser; this.acquireTimeoutMillis = acquireTimeoutMillis; - this.renewerFactory = renewerFactory; + this.scheduler = checkNotNull(scheduler); + this.workers = checkNotNull(workers); this.nanoTime = nanoTime; } - private static ScheduledExecutorService createRenewer() { + private static ScheduledExecutorService createRenewalScheduler() { ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor( - 1, new ExecutorThreadFactory("rest-commit-lock-renew")); + 1, new ExecutorThreadFactory("rest-commit-lock-timer")); executor.setRemoveOnCancelPolicy(true); return executor; } @@ -161,25 +171,51 @@ public void ensureValid() { } private class Lease implements AutoCloseable { - private final ScheduledExecutorService executor; private final ScheduledFuture renewal; private final AtomicBoolean closed = new AtomicBoolean(); + private final AtomicBoolean renewing = new AtomicBoolean(); private final long leaseMillis; private volatile long renewedAt; - @Nullable private volatile Exception renewalFailure; + @Nullable private volatile Throwable renewalFailure; + @Nullable private volatile FutureTask renewalRequest; private Lease(long leaseMillis, long requestedAt) { this.leaseMillis = leaseMillis; this.renewedAt = requestedAt; - this.executor = renewerFactory.get(); long interval = Math.max(1, leaseMillis / 3); + this.renewal = + scheduler.scheduleWithFixedDelay( + this::submitRenewal, interval, interval, TimeUnit.MILLISECONDS); + } + + private void submitRenewal() { + if (closed.get() || renewalFailure != null || !renewing.compareAndSet(false, true)) { + return; + } + FutureTask request = + new FutureTask<>( + () -> { + try { + renew(); + } finally { + renewing.set(false); + } + }, + null); + renewalRequest = request; + // Closing can race with dispatch. Publish the request before rechecking closed so + // either close or this dispatcher cancels it before it can start another RPC. + if (closed.get()) { + request.cancel(true); + renewing.set(false); + return; + } try { - this.renewal = - executor.scheduleWithFixedDelay( - this::renew, interval, interval, TimeUnit.MILLISECONDS); + workers.execute(request); } catch (RuntimeException | Error e) { - Lock.release(executor::shutdownNow, e); - throw e; + renewalFailure = e; + request.cancel(false); + renewing.set(false); } } @@ -201,7 +237,7 @@ private void renew() { "The commit lease duration changed during renewal."); } renewedAt = requestedAt; - } catch (Exception e) { + } catch (Exception | Error e) { renewalFailure = e; } } @@ -222,9 +258,12 @@ public void ensureValid() { public void close() { if (closed.compareAndSet(false, true)) { try { - renewal.cancel(true); + renewal.cancel(false); } finally { - executor.shutdownNow(); + FutureTask request = renewalRequest; + if (request != null) { + request.cancel(true); + } } } } @@ -232,6 +271,6 @@ public void close() { @Override public void close() { - // Lease scopes own their renewal workers; the RESTApi has no owned closeable resources. + // Lease scopes cancel their own tasks; shared executors outlive individual locks. } } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java index c92d147c6f78..7c9181618d42 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java @@ -30,19 +30,26 @@ import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -56,12 +63,13 @@ class RESTCatalogLockTest { private final RESTApi api = mock(RESTApi.class); private final ScheduledExecutorService renewer = mock(ScheduledExecutorService.class); + private final ExecutorService workers = mock(ExecutorService.class); private final ScheduledFuture future = mock(ScheduledFuture.class); private final Identifier identifier = new Identifier("db", "table", "dev"); private final AtomicLong clock = new AtomicLong(); private final RESTCatalogLock lock = new RESTCatalogLock( - api, identifier, "table-id", "morax-job", 0, () -> renewer, clock::get); + api, identifier, "table-id", "morax-job", 0, renewer, workers, clock::get); @Test void callbackIsProtectedAndLeaseEndsOnReturn() throws Exception { @@ -73,7 +81,7 @@ void callbackIsProtectedAndLeaseEndsOnReturn() throws Exception { return "published"; })) .isEqualTo("published"); - verify(future).cancel(true); + verify(future).cancel(false); assertThatThrownBy(lock::ensureValid).hasMessageContaining("No commit lease"); } @@ -88,7 +96,7 @@ void callbackFailureStopsRenewal() { throw failure; })) .isSameAs(failure); - verify(future).cancel(true); + verify(future).cancel(false); } @Test @@ -106,7 +114,7 @@ void renewalUsesExactOwnerAndStopsAfterClose() throws Exception { }); callback.run(); verify(api).renewCommitLock(identifier, "table-id", "morax-job"); - verify(future).cancel(true); + verify(future).cancel(false); } @Test @@ -239,13 +247,13 @@ void slowAcquisitionCannotStartProtectedOperation() { })) .isInstanceOf(Lock.AcquisitionException.class) .hasRootCauseMessage("The commit lease has expired locally."); - verify(future).cancel(true); - verify(renewer).shutdownNow(); + verify(future).cancel(false); + verify(renewer, never()).shutdownNow(); assertThatThrownBy(lock::ensureValid).hasMessageContaining("No commit lease"); } @Test - void schedulerFailureStopsWorkerAndAllowsFreshAcquisition() throws Exception { + void schedulerFailureKeepsSharedExecutorsUsableForFreshAcquisition() throws Exception { grant("morax-job", 60000); RejectedExecutionException failure = new RejectedExecutionException("Stopped worker"); when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) @@ -258,22 +266,22 @@ void schedulerFailureStopsWorkerAndAllowsFreshAcquisition() throws Exception { })) .isInstanceOf(Lock.AcquisitionException.class) .hasCause(failure); - verify(renewer).shutdownNow(); + verify(renewer, never()).shutdownNow(); assertThatThrownBy(lock::ensureValid).hasMessageContaining("No commit lease"); doReturn(future) .when(renewer) .scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any()); assertThat(run(() -> "published")).isEqualTo("published"); - verify(renewer, times(2)).shutdownNow(); + verify(renewer, never()).shutdownNow(); } @ParameterizedTest @ValueSource(booleans = {false, true}) - void workerShutdownFailurePreservesOperationOutcome(boolean failOperation) throws Exception { + void taskCancellationFailurePreservesOperationOutcome(boolean failOperation) throws Exception { grant("morax-job", 60000); - RuntimeException releaseFailure = new RuntimeException("Worker shutdown failed"); + RuntimeException releaseFailure = new RuntimeException("Task cancellation failed"); RuntimeException operationFailure = new RuntimeException("Preparation failed"); - doThrow(releaseFailure).when(renewer).shutdownNow(); + doThrow(releaseFailure).when(future).cancel(false); Callable operation = () -> { if (failOperation) { @@ -288,7 +296,7 @@ void workerShutdownFailurePreservesOperationOutcome(boolean failOperation) throw } else { assertThat(run(operation)).isEqualTo("published"); } - verify(future).cancel(true); + verify(future).cancel(false); assertThatThrownBy(lock::ensureValid).hasMessageContaining("No commit lease"); } @@ -318,30 +326,258 @@ void invalidRenewalGrantCannotExtendLease(boolean differentOwner) throws Excepti } @Test - void eachScopeOwnsAndStopsItsRenewalWorker() throws Exception { + void closingAScopeKeepsSharedRenewalExecutorsUsable() throws Exception { grant("morax-job", 60000); - ScheduledExecutorService secondWorker = mock(ScheduledExecutorService.class); ScheduledFuture secondRenewal = mock(ScheduledFuture.class); - doReturn(secondRenewal) - .when(secondWorker) - .scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any()); - AtomicInteger workers = new AtomicInteger(); - RESTCatalogLock scopes = + when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) + .thenAnswer(invocation -> future) + .thenAnswer(invocation -> secondRenewal); + assertThat(run(() -> "first")).isEqualTo("first"); + assertThat(run(() -> "second")).isEqualTo("second"); + verify(future).cancel(false); + verify(secondRenewal).cancel(false); + verify(renewer, never()).shutdownNow(); + verify(workers, never()).shutdownNow(); + } + + @Test + void queuedRenewalIsCancelledAndCannotAffectANewScope() throws Exception { + grant("morax-job", 60000); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenReturn(new CommitLockResponse(true, "morax-job", 100000, 60000, null)); + List requests = queueRenewals(); + AutoCloseable first = lock.acquire(); + Runnable oldTick = renewal(); + oldTick.run(); + oldTick.run(); + assertThat(requests).hasSize(1); + first.close(); + requests.get(0).run(); + verify(api, never()).renewCommitLock(identifier, "table-id", "morax-job"); + try (AutoCloseable second = lock.acquire()) { + first.close(); + oldTick.run(); + assertThat(requests).hasSize(1); + renewal(2).run(); + assertThat(requests).hasSize(2); + requests.get(1).run(); + lock.ensureValid(); + } + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); + verify(workers, never()).shutdownNow(); + } + + @Test + void queuedRenewalCannotReviveALocallyExpiredLease() throws Exception { + grant("morax-job", 60000); + List requests = queueRenewals(); + run( + () -> { + renewal().run(); + clock.set(TimeUnit.MILLISECONDS.toNanos(60000)); + requests.get(0).run(); + assertThatThrownBy(lock::ensureValid) + .hasRootCauseMessage("The commit lease has expired locally."); + return null; + }); + verify(api, never()).renewCommitLock(identifier, "table-id", "morax-job"); + } + + @Test + void workerDispatchFailureInvalidatesOnlyItsScope() throws Exception { + grant("morax-job", 60000); + RejectedExecutionException failure = new RejectedExecutionException("Dispatch rejected"); + doThrow(failure).when(workers).execute(any(Runnable.class)); + run( + () -> { + renewal().run(); + assertThatThrownBy(lock::ensureValid).hasCause(failure); + return null; + }); + grant("morax-job", 60000); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenReturn(new CommitLockResponse(true, "morax-job", 100000, 60000, null)); + run( + () -> { + renewal(2).run(); + lock.ensureValid(); + return null; + }); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); + verify(workers, never()).shutdownNow(); + } + + @Test + void renewalErrorInvalidatesLeaseAndStopsFurtherDispatch() throws Exception { + grant("morax-job", 60000); + AssertionError failure = new AssertionError("RPC failed"); + when(api.renewCommitLock(identifier, "table-id", "morax-job")).thenThrow(failure); + run( + () -> { + Runnable tick = renewal(); + tick.run(); + tick.run(); + assertThatThrownBy(lock::ensureValid).hasCause(failure); + return null; + }); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void lateOldRenewalCannotChangeANewScope(boolean failOldRenewal) throws Exception { + grant("morax-job", 60000); + List requests = queueRenewals(); + CountDownLatch started = new CountDownLatch(1); + CountDownLatch resume = new CountDownLatch(1); + CommitLockResponse renewed = new CommitLockResponse(true, "morax-job", 100000, 60000, null); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenAnswer( + invocation -> { + started.countDown(); + awaitIgnoringInterrupts(resume); + if (failOldRenewal) { + throw new IllegalStateException("Old renewal failed"); + } + return renewed; + }) + .thenReturn(renewed); + ExecutorService runner = Executors.newSingleThreadExecutor(); + try (AutoCloseable first = lock.acquire()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(20000)); + renewal().run(); + Future oldResponse = runner.submit(requests.get(0)); + assertThat(started.await(10, TimeUnit.SECONDS)).isTrue(); + first.close(); + try (AutoCloseable second = lock.acquire()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(30000)); + renewal(2).run(); + requests.get(1).run(); + resume.countDown(); + oldResponse.get(10, TimeUnit.SECONDS); + // Only the new scope's renewal at 30s keeps it valid beyond 80s. + clock.set(TimeUnit.MILLISECONDS.toNanos(85000)); + lock.ensureValid(); + } + } finally { + resume.countDown(); + runner.shutdownNow(); + assertThat(runner.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + verify(api, times(2)).renewCommitLock(identifier, "table-id", "morax-job"); + } + + @Test + void lateSuccessfulRenewalCannotReviveAnExpiredScope() throws Exception { + grant("morax-job", 60000); + List requests = queueRenewals(); + CountDownLatch started = new CountDownLatch(1); + CountDownLatch resume = new CountDownLatch(1); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenAnswer( + invocation -> { + started.countDown(); + assertThat(resume.await(10, TimeUnit.SECONDS)).isTrue(); + return new CommitLockResponse(true, "morax-job", 100000, 60000, null); + }); + ExecutorService runner = Executors.newSingleThreadExecutor(); + try (AutoCloseable scope = lock.acquire()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(20000)); + renewal().run(); + Future response = runner.submit(requests.get(0)); + assertThat(started.await(10, TimeUnit.SECONDS)).isTrue(); + clock.set(TimeUnit.MILLISECONDS.toNanos(60000)); + assertThatThrownBy(lock::ensureValid) + .hasRootCauseMessage("The commit lease has expired locally."); + resume.countDown(); + response.get(10, TimeUnit.SECONDS); + assertThatThrownBy(lock::ensureValid) + .hasRootCauseMessage("The commit lease has expired locally."); + } finally { + resume.countDown(); + runner.shutdownNow(); + assertThat(runner.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void blockedRenewalDoesNotBlockAnotherLeaseAndCloseInterruptsOnlyItsRequest() throws Exception { + grant("morax-job", 60000); + ScheduledFuture secondRenewal = mock(ScheduledFuture.class); + when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) + .thenAnswer(invocation -> future) + .thenAnswer(invocation -> secondRenewal); + when(api.acquireCommitLock(identifier, "table-id", "other-job")) + .thenReturn(new CommitLockResponse(true, "other-job", 100000, 60000, null)); + CountDownLatch firstStarted = new CountDownLatch(1); + CountDownLatch resumeFirst = new CountDownLatch(1); + CountDownLatch firstInterrupted = new CountDownLatch(1); + CountDownLatch secondRenewed = new CountDownLatch(1); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenAnswer( + invocation -> { + firstStarted.countDown(); + try { + assertThat(resumeFirst.await(10, TimeUnit.SECONDS)).isTrue(); + } catch (InterruptedException e) { + firstInterrupted.countDown(); + throw e; + } + return new CommitLockResponse(true, "morax-job", 100000, 60000, null); + }); + when(api.renewCommitLock(identifier, "table-id", "other-job")) + .thenAnswer( + invocation -> { + secondRenewed.countDown(); + return new CommitLockResponse(true, "other-job", 100000, 60000, null); + }); + ExecutorService sharedWorkers = Executors.newCachedThreadPool(); + RESTCatalogLock firstLock = new RESTCatalogLock( api, identifier, "table-id", "morax-job", 0, - () -> workers.getAndIncrement() == 0 ? renewer : secondWorker, + renewer, + sharedWorkers, + clock::get); + RESTCatalogLock secondLock = + new RESTCatalogLock( + api, + identifier, + "table-id", + "other-job", + 0, + renewer, + sharedWorkers, clock::get); - assertThat(scopes.runWithLock(() -> "first")).isEqualTo("first"); - assertThat(scopes.runWithLock(() -> "second")).isEqualTo("second"); - assertThat(workers.get()).isEqualTo(2); - verify(future).cancel(true); - verify(secondRenewal).cancel(true); - verify(renewer).shutdownNow(); - verify(secondWorker).shutdownNow(); + try (AutoCloseable first = firstLock.acquire(); + AutoCloseable second = secondLock.acquire()) { + ArgumentCaptor ticks = ArgumentCaptor.forClass(Runnable.class); + verify(renewer, times(2)) + .scheduleWithFixedDelay( + ticks.capture(), eq(20000L), eq(20000L), eq(TimeUnit.MILLISECONDS)); + Runnable firstTick = ticks.getAllValues().get(0); + firstTick.run(); + assertThat(firstStarted.await(10, TimeUnit.SECONDS)).isTrue(); + firstTick.run(); + firstTick.run(); + ticks.getAllValues().get(1).run(); + assertThat(secondRenewed.await(10, TimeUnit.SECONDS)).isTrue(); + first.close(); + assertThat(firstInterrupted.await(10, TimeUnit.SECONDS)).isTrue(); + assertThat(sharedWorkers.isShutdown()).isFalse(); + secondLock.ensureValid(); + firstTick.run(); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); + verify(api).renewCommitLock(identifier, "table-id", "other-job"); + verify(secondRenewal, never()).cancel(anyBoolean()); + } finally { + resumeFirst.countDown(); + sharedWorkers.shutdownNow(); + assertThat(sharedWorkers.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } } @Test @@ -351,7 +587,7 @@ void nestedPublicationReusesLeaseAndRechecksValidity() throws Exception { assertThat(publication.runWithLock(() -> publication.runWithLock(() -> "published"))) .isEqualTo("published"); verify(api).acquireCommitLock(identifier, "table-id", "morax-job"); - verify(future).cancel(true); + verify(future).cancel(false); publication.runWithLock( () -> { clock.addAndGet(TimeUnit.MILLISECONDS.toNanos(60000)); @@ -377,7 +613,7 @@ void closingAnOldScopeTwiceCannotInvalidateTheNewLease() throws Exception { lock.ensureValid(); } verify(api, times(2)).acquireCommitLock(identifier, "table-id", "morax-job"); - verify(future, times(2)).cancel(true); + verify(future, times(2)).cancel(false); } @Test @@ -387,7 +623,14 @@ void interruptedContentionStopsAcquisition() { .thenReturn(CommitLockResponse.unavailable()); RESTCatalogLock waiting = new RESTCatalogLock( - api, identifier, "table-id", "morax-job", 1000, () -> renewer, clock::get); + api, + identifier, + "table-id", + "morax-job", + 1000, + renewer, + workers, + clock::get); Thread.currentThread().interrupt(); try { assertThatThrownBy( @@ -409,6 +652,13 @@ private T run(Callable callable) throws Exception { } private void grant(String owner, long leaseMillis) { + doAnswer( + invocation -> { + ((Runnable) invocation.getArgument(0)).run(); + return null; + }) + .when(workers) + .execute(any(Runnable.class)); when(api.acquireCommitLock(identifier, "table-id", "morax-job")) .thenReturn(new CommitLockResponse(true, owner, 100000, leaseMillis, null)); when(renewer.scheduleWithFixedDelay(any(Runnable.class), anyLong(), anyLong(), any())) @@ -416,10 +666,37 @@ private void grant(String owner, long leaseMillis) { } private Runnable renewal() { + return renewal(1); + } + + private Runnable renewal(int scopes) { ArgumentCaptor callback = ArgumentCaptor.forClass(Runnable.class); - verify(renewer) + verify(renewer, times(scopes)) .scheduleWithFixedDelay( callback.capture(), eq(20000L), eq(20000L), eq(TimeUnit.MILLISECONDS)); return callback.getValue(); } + + private List queueRenewals() { + List requests = new ArrayList<>(); + doAnswer( + invocation -> { + requests.add(invocation.getArgument(0)); + return null; + }) + .when(workers) + .execute(any(Runnable.class)); + return requests; + } + + private static void awaitIgnoringInterrupts(CountDownLatch latch) { + while (true) { + try { + assertThat(latch.await(10, TimeUnit.SECONDS)).isTrue(); + return; + } catch (InterruptedException ignored) { + // Emulate a transport that completes its RPC despite scope cancellation. + } + } + } } From 828175cc2c704cdfa7e30b687ef3a6d522dd7f92 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Tue, 13 Oct 2026 14:34:11 +0800 Subject: [PATCH 7/9] [rest] Guard rollback with snapshot UUID and writer identity --- docs/docs/concepts/concurrency-control.md | 15 +++-- docs/static/rest-catalog-open-api.yaml | 22 ++++--- .../java/org/apache/paimon/rest/RESTApi.java | 7 ++- .../rest/requests/RollbackTableRequest.java | 15 ++++- .../org/apache/paimon/AbstractFileStore.java | 4 +- .../paimon/catalog/AbstractCatalog.java | 1 + .../org/apache/paimon/catalog/Catalog.java | 6 +- .../paimon/catalog/DelegateCatalog.java | 4 +- .../apache/paimon/catalog/TableRollback.java | 3 +- .../operation/commit/CommitRollback.java | 6 +- .../org/apache/paimon/rest/RESTCatalog.java | 4 +- .../paimon/table/CatalogEnvironment.java | 12 +++- .../apache/paimon/tag/SnapshotLoaderImpl.java | 2 +- .../operation/commit/CommitRollbackTest.java | 60 +++++++++++++++++++ .../paimon/rest/CommitLockProtocolTest.java | 6 +- .../apache/paimon/rest/MockRESTMessage.java | 24 +++++++- .../apache/paimon/rest/RESTCatalogServer.java | 20 ++----- .../apache/paimon/rest/RESTCatalogTest.java | 17 +++++- .../rest/RESTCommitLockIntegrationTest.java | 6 +- .../paimon/rest/RollbackProtocolTest.java | 5 +- .../paimon/table/CatalogEnvironmentTest.java | 25 +++++--- .../table/FallbackReadFileStoreTableTest.java | 2 + .../paimon/tag/SnapshotLoaderImplTest.java | 11 +++- 23 files changed, 214 insertions(+), 63 deletions(-) create mode 100644 paimon-core/src/test/java/org/apache/paimon/operation/commit/CommitRollbackTest.java diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index a925a7a41f96..6bc8f8dc3cf3 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -134,10 +134,17 @@ respect another writer's active lease. The server uses its own persisted table p decision, rather than client options. Capability changes must be serialized with publication and lease acquisition; disabling enforcement must not bypass an active lease. -Rollback inside a lease supplies `tableId` and `commitUser` in addition to the expected -`fromSnapshot`. The server must validate the lease and expected head, roll back and clear the lease -atomically. Ordinary rollback still supplies a known table UUID but omits `commitUser`; it cannot -change a leased head. The UUID is omitted only when the target table or branch identity is unknown. +Automatic conflict rollback always supplies the runtime branch and writer's `commitUser`, together +with the expected head's `fromSnapshot` and `fromSnapshotUuid`. Snapshot IDs can be reused after a +rollback, so the server must check both fields against the current head inside the mutation +transaction, before changing partitions or deleting files. Automatic rollback skips legacy snapshots +without a UUID. A known table or branch UUID is supplied independently of writer identity. + +`commitUser` identifies the writer; it does not require a lease to exist. If a live lease exists, the +writer and authenticated caller must match its owner. Without a live lease, conditional optimistic +rollback is allowed. The server must validate admission and the expected head, roll back and clear +the lease atomically. Manual rollback can omit writer identity, but cannot change a leased head. +The table UUID is omitted only when the target table or branch identity is unknown. This is a commit admission lease, not a generation or fencing token. Requests with the same commit user and authenticated caller share ownership. Use a unique commit user per logical writer diff --git a/docs/static/rest-catalog-open-api.yaml b/docs/static/rest-catalog-open-api.yaml index fb62fd483149..276da1ce8261 100644 --- a/docs/static/rest-catalog-open-api.yaml +++ b/docs/static/rest-catalog-open-api.yaml @@ -3661,24 +3661,32 @@ components: type: object description: >- tableId identifies the expected table incarnation independently of lock ownership. - For rollback within a writer's commit lock, also supply commitUser and fromSnapshot so - the server can validate the expected head and owner. Omit commitUser when no writer - lock is used, and omit tableId only when its UUID is unavailable. - Within a lease, the server must validate table incarnation, current head, authenticated - caller and unexpired lease ownership, then roll back and clear the lease atomically. - A different owner or a request without ownership cannot change the head under a lease. + Writer rollback supplies commitUser, fromSnapshot and fromSnapshotUuid regardless of + whether the writer acquired a lease. commitUser identifies the writer, not a requirement + for a lease to exist. A live lease requires the exact owner and authenticated caller; + without a live lease, conditional optimistic rollback is allowed. Snapshot IDs can be + reused after rollback, so both expected head fields must be checked against the current + head inside the mutation transaction before partition changes or file cleanup. The server + must validate table incarnation, admission and expected head, then roll back and clear + the lease atomically. Manual rollback may omit writer identity, but cannot change a + leased head. Omit tableId only when its UUID is unavailable. properties: instant: $ref: '#/components/schemas/Instant' fromSnapshot: type: [ integer, "null" ] format: int64 + description: Expected latest snapshot ID, required for writer rollback. + fromSnapshotUuid: + type: [ string, "null" ] + minLength: 1 + description: Expected latest snapshot UUID; requires fromSnapshot and is required for writer rollback. tableId: type: [ string, "null" ] description: Expected table or branch UUID, independently of commitUser. commitUser: type: [ string, "null" ] - description: Exact owner of the writer's lock. + description: Writer identity for admission against any active lease; requires both expected head fields. RollbackSchemaRequest: type: object required: diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java index d674b03915d5..f97425a16b89 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java @@ -763,7 +763,8 @@ public CommitLockResponse renewCommitLock( * @param instant instant to rollback * @param fromSnapshot snapshot from, success only occurs when the latest snapshot is this * snapshot. - * @param commitUser owner of the writer lock, or null without a writer lock + * @param fromSnapshotUuid expected snapshot UUID, checked together with fromSnapshot + * @param commitUser writer identity for active-lock admission, or null without a writer * @throws NoSuchResourceException Exception thrown on HTTP 404 means the table or the snapshot * or the tag not exists * @throws ForbiddenException Exception thrown on HTTP 403 means don't have the permission for @@ -774,9 +775,11 @@ public void rollbackTo( @Nullable String tableUuid, Instant instant, @Nullable Long fromSnapshot, + @Nullable String fromSnapshotUuid, @Nullable String commitUser) { RollbackTableRequest request = - new RollbackTableRequest(instant, fromSnapshot, tableUuid, commitUser); + new RollbackTableRequest( + instant, fromSnapshot, fromSnapshotUuid, tableUuid, commitUser); client.post( resourcePaths.rollbackTable( identifier.getDatabaseName(), identifier.getObjectName()), diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java b/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java index 77fc97049c95..b76747ef2ad8 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/requests/RollbackTableRequest.java @@ -46,6 +46,11 @@ public class RollbackTableRequest implements RESTRequest { @Nullable private final Long fromSnapshot; + @JsonProperty("fromSnapshotUuid") + @JsonInclude(JsonInclude.Include.NON_NULL) + @Nullable + private final String fromSnapshotUuid; + @JsonProperty("tableId") @JsonInclude(JsonInclude.Include.NON_NULL) @Nullable @@ -57,14 +62,16 @@ public class RollbackTableRequest implements RESTRequest { private final String commitUser; @JsonCreator - @ConstructorProperties({"instant", "fromSnapshot", "tableId", "commitUser"}) + @ConstructorProperties({"instant", "fromSnapshot", "fromSnapshotUuid", "tableId", "commitUser"}) public RollbackTableRequest( @JsonProperty(FIELD_INSTANT) Instant instant, @JsonProperty(FIELD_FROM_SNAPSHOT) @Nullable Long fromSnapshot, + @JsonProperty("fromSnapshotUuid") @Nullable String fromSnapshotUuid, @JsonProperty("tableId") @Nullable String tableId, @JsonProperty("commitUser") @Nullable String commitUser) { this.instant = instant; this.fromSnapshot = fromSnapshot; + this.fromSnapshotUuid = fromSnapshotUuid; this.tableId = tableId; this.commitUser = commitUser; } @@ -91,4 +98,10 @@ public Instant getInstant() { public Long getFromSnapshot() { return fromSnapshot; } + + @JsonGetter("fromSnapshotUuid") + @Nullable + public String getFromSnapshotUuid() { + return fromSnapshotUuid; + } } diff --git a/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java b/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java index fbd43c960ff5..15ae1627ad12 100644 --- a/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java +++ b/paimon-core/src/main/java/org/apache/paimon/AbstractFileStore.java @@ -308,9 +308,7 @@ public FileStoreCommitImpl newCommit(String commitUser, FileStoreTable table) { scanner); CommitRollback rollback = null; TableRollback tableRollback = - options.restCommitAcquireLock() - ? catalogEnvironment.catalogTableRollback(options.branch(), commitUser) - : catalogEnvironment.catalogTableRollback(null, null); + catalogEnvironment.catalogTableRollback(options.branch(), commitUser); if (tableRollback != null) { rollback = new CommitRollback(tableRollback); } diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/AbstractCatalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/AbstractCatalog.java index d9503d43d93b..49eabacbf06d 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/AbstractCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/AbstractCatalog.java @@ -717,6 +717,7 @@ public void rollbackTo( @Nullable String tableUuid, Instant instant, @Nullable Long fromSnapshot, + @Nullable String fromSnapshotUuid, @Nullable String commitUser) throws Catalog.TableNotExistException { throw new UnsupportedOperationException(); diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java index 02e318a13ee5..beb43b59d5e2 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java @@ -715,7 +715,7 @@ default boolean supportsListTableByType() { *
    *
  • {@link #commitSnapshot(Identifier, String, String, Snapshot, List)}. *
  • {@link #loadSnapshot(Identifier)}. - *
  • {@link #rollbackTo(Identifier, String, Instant, Long, String)}. + *
  • {@link #rollbackTo(Identifier, String, Instant, Long, String, String)}. *
  • {@link #createBranch(Identifier, String, String)}. *
  • {@link #dropBranch(Identifier, String)}. *
  • {@link #renameBranch(Identifier, String, String)}. @@ -847,7 +847,8 @@ default void resetConsumer( * @param instant like snapshotId or tagName * @param fromSnapshot snapshot from, success only occurs when the latest snapshot is this * snapshot - * @param commitUser owner of the writer's lock, or null when no writer lock is used + * @param fromSnapshotUuid expected head UUID; distinguishes snapshots that reuse the same ID + * @param commitUser writer identity for active-lock admission, or null when unavailable * @throws Catalog.TableNotExistException if the table does not exist * @throws UnsupportedOperationException if the catalog does not {@link * #supportsVersionManagement()} @@ -857,6 +858,7 @@ void rollbackTo( @Nullable String tableUuid, Instant instant, @Nullable Long fromSnapshot, + @Nullable String fromSnapshotUuid, @Nullable String commitUser) throws Catalog.TableNotExistException; diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java index 89159783fbf2..1aeeff926d07 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/DelegateCatalog.java @@ -254,9 +254,11 @@ public void rollbackTo( @Nullable String tableUuid, Instant instant, @Nullable Long fromSnapshot, + @Nullable String fromSnapshotUuid, @Nullable String commitUser) throws Catalog.TableNotExistException { - wrapped.rollbackTo(identifier, tableUuid, instant, fromSnapshot, commitUser); + wrapped.rollbackTo( + identifier, tableUuid, instant, fromSnapshot, fromSnapshotUuid, commitUser); } @Override diff --git a/paimon-core/src/main/java/org/apache/paimon/catalog/TableRollback.java b/paimon-core/src/main/java/org/apache/paimon/catalog/TableRollback.java index 5cb2a521924d..bbe16ec3bbec 100644 --- a/paimon-core/src/main/java/org/apache/paimon/catalog/TableRollback.java +++ b/paimon-core/src/main/java/org/apache/paimon/catalog/TableRollback.java @@ -25,5 +25,6 @@ /** Rollback table to instant from snapshot. */ public interface TableRollback { - void rollbackTo(Instant instant, @Nullable Long fromSnapshot); + void rollbackTo( + Instant instant, @Nullable Long fromSnapshot, @Nullable String fromSnapshotUuid); } diff --git a/paimon-core/src/main/java/org/apache/paimon/operation/commit/CommitRollback.java b/paimon-core/src/main/java/org/apache/paimon/operation/commit/CommitRollback.java index 683b6555a651..ead30bc8f878 100644 --- a/paimon-core/src/main/java/org/apache/paimon/operation/commit/CommitRollback.java +++ b/paimon-core/src/main/java/org/apache/paimon/operation/commit/CommitRollback.java @@ -32,10 +32,12 @@ public CommitRollback(TableRollback tableRollback) { } public boolean tryToRollback(Snapshot latestSnapshot) { - if (latestSnapshot.commitKind() == Snapshot.CommitKind.COMPACT) { + if (latestSnapshot.commitKind() == Snapshot.CommitKind.COMPACT + && latestSnapshot.uuid() != null) { long latest = latestSnapshot.id(); try { - tableRollback.rollbackTo(Instant.snapshot(latest - 1), latest); + tableRollback.rollbackTo( + Instant.snapshot(latest - 1), latest, latestSnapshot.uuid()); return true; } catch (Exception ignored) { } diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java index 1755f6017155..07e4a4ae18f8 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java @@ -562,10 +562,12 @@ public void rollbackTo( @Nullable String tableUuid, Instant instant, @Nullable Long fromSnapshot, + @Nullable String fromSnapshotUuid, @Nullable String commitUser) throws Catalog.TableNotExistException { try { - api.rollbackTo(identifier, tableUuid, instant, fromSnapshot, commitUser); + api.rollbackTo( + identifier, tableUuid, instant, fromSnapshot, fromSnapshotUuid, commitUser); } catch (NoSuchResourceException e) { if (StringUtils.equals(e.resourceType(), ErrorResponse.RESOURCE_TYPE_SNAPSHOT)) { throw new IllegalArgumentException( diff --git a/paimon-core/src/main/java/org/apache/paimon/table/CatalogEnvironment.java b/paimon-core/src/main/java/org/apache/paimon/table/CatalogEnvironment.java index 5988ee128f1c..7cea98df0069 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/CatalogEnvironment.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/CatalogEnvironment.java @@ -180,7 +180,7 @@ private Lock createFactoryLock( lockContext, lockIdentifier, uuid, commitUser, tableOptions); } - /** Create rollback with the table incarnation and optional writer lock context. */ + /** Create rollback with the table incarnation and optional writer identity. */ @Nullable public TableRollback catalogTableRollback( @Nullable String branch, @Nullable String commitUser) { @@ -193,9 +193,15 @@ public TableRollback catalogTableRollback( identifier.getDatabaseName(), identifier.getTableName(), branch); - return (instant, fromSnapshot) -> { + return (instant, fromSnapshot, fromSnapshotUuid) -> { try { - catalog.rollbackTo(rollbackIdentifier, uuid, instant, fromSnapshot, commitUser); + catalog.rollbackTo( + rollbackIdentifier, + uuid, + instant, + fromSnapshot, + fromSnapshotUuid, + commitUser); } catch (Catalog.TableNotExistException e) { throw new RuntimeException(e); } diff --git a/paimon-core/src/main/java/org/apache/paimon/tag/SnapshotLoaderImpl.java b/paimon-core/src/main/java/org/apache/paimon/tag/SnapshotLoaderImpl.java index 8a47e88b1b93..3aab3235aec8 100644 --- a/paimon-core/src/main/java/org/apache/paimon/tag/SnapshotLoaderImpl.java +++ b/paimon-core/src/main/java/org/apache/paimon/tag/SnapshotLoaderImpl.java @@ -59,7 +59,7 @@ public Optional load() throws IOException { @Override public void rollback(Instant instant) throws IOException { try (Catalog catalog = catalogLoader.load()) { - catalog.rollbackTo(identifier, uuid, instant, null, null); + catalog.rollbackTo(identifier, uuid, instant, null, null, null); } catch (RuntimeException e) { throw e; } catch (Exception e) { diff --git a/paimon-core/src/test/java/org/apache/paimon/operation/commit/CommitRollbackTest.java b/paimon-core/src/test/java/org/apache/paimon/operation/commit/CommitRollbackTest.java new file mode 100644 index 000000000000..6e0c4e50dc97 --- /dev/null +++ b/paimon-core/src/test/java/org/apache/paimon/operation/commit/CommitRollbackTest.java @@ -0,0 +1,60 @@ +/* + * 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. + */ + +package org.apache.paimon.operation.commit; + +import org.apache.paimon.Snapshot; +import org.apache.paimon.catalog.TableRollback; +import org.apache.paimon.table.Instant; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +class CommitRollbackTest { + + @Test + void legacyCompactionWithoutUuidCannotBeRolledBackAutomatically() { + Snapshot head = mock(Snapshot.class); + when(head.commitKind()).thenReturn(Snapshot.CommitKind.COMPACT); + TableRollback rollback = mock(TableRollback.class); + + assertThat(new CommitRollback(rollback).tryToRollback(head)).isFalse(); + verifyNoInteractions(rollback); + } + + @Test + void changedHeadRejectsRollbackAndLeavesTheConflictUnresolved() { + Snapshot head = mock(Snapshot.class); + when(head.commitKind()).thenReturn(Snapshot.CommitKind.COMPACT); + when(head.id()).thenReturn(6L); + when(head.uuid()).thenReturn("head-uuid"); + TableRollback rollback = mock(TableRollback.class); + doThrow(new IllegalStateException("Head UUID changed")) + .when(rollback) + .rollbackTo(any(Instant.class), eq(6L), eq("head-uuid")); + + assertThat(new CommitRollback(rollback).tryToRollback(head)).isFalse(); + } +} diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java index b4aee172efd7..5c046d80f85b 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/CommitLockProtocolTest.java @@ -131,7 +131,8 @@ public CatalogLoader catalogLoader() { }; Identifier identifier = new Identifier("db", "table", "dev"); server.enqueue(json("{}")); - catalog.rollbackTo(identifier, "table-id", Instant.snapshot(1L), 2L, "morax-job"); + catalog.rollbackTo( + identifier, "table-id", Instant.snapshot(1L), 2L, "head-uuid", "morax-job"); RecordedRequest owned = server.takeRequest(10, TimeUnit.SECONDS); assertThat(owned.getPath()).endsWith("/rollback"); RollbackTableRequest request = @@ -140,13 +141,14 @@ public CatalogLoader catalogLoader() { assertThat(request.getCommitUser()).isEqualTo("morax-job"); assertThat(request.getFromSnapshot()).isEqualTo(2L); server.enqueue(json("{}")); - catalog.rollbackTo(identifier, null, Instant.snapshot(1L), 2L, null); + catalog.rollbackTo(identifier, null, Instant.snapshot(1L), 2L, null, null); ObjectNode legacy = RESTApi.fromJson( server.takeRequest(10, TimeUnit.SECONDS).getBody().readUtf8(), ObjectNode.class); assertThat(legacy.has("tableId")).isFalse(); assertThat(legacy.has("commitUser")).isFalse(); + assertThat(legacy.has("fromSnapshotUuid")).isFalse(); } } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTMessage.java b/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTMessage.java index dafeae833a03..caa1e5c08d56 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTMessage.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/MockRESTMessage.java @@ -52,6 +52,7 @@ import org.apache.paimon.schema.Schema; import org.apache.paimon.schema.SchemaChange; import org.apache.paimon.table.Instant; +import org.apache.paimon.table.TableSnapshot; import org.apache.paimon.types.DataField; import org.apache.paimon.types.DataType; import org.apache.paimon.types.DataTypes; @@ -66,6 +67,8 @@ import org.apache.paimon.shade.guava30.com.google.common.collect.Lists; import org.apache.paimon.shade.guava30.com.google.common.collect.Maps; +import javax.annotation.Nullable; + import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -79,6 +82,23 @@ /** Mock REST message. */ public class MockRESTMessage { + public static void validateRollbackHead( + RollbackTableRequest request, @Nullable TableSnapshot current) { + if (request.getFromSnapshot() != null + && (current == null || request.getFromSnapshot() != current.snapshot().id())) { + throw new IllegalStateException( + String.format( + "Latest snapshot %s is not %s", + current == null ? null : current.snapshot().id(), + request.getFromSnapshot())); + } + if (request.getFromSnapshotUuid() != null + && (current == null + || !request.getFromSnapshotUuid().equals(current.snapshot().uuid()))) { + throw new IllegalStateException("Latest snapshot UUID changed"); + } + } + public static String databaseName() { return "database"; } @@ -274,11 +294,11 @@ public static GetTableTokenResponse getTableCredentialsResponse() { } public static RollbackTableRequest rollbackTableRequestBySnapshot(long snapshotId) { - return new RollbackTableRequest(Instant.snapshot(snapshotId), null, null, null); + return new RollbackTableRequest(Instant.snapshot(snapshotId), null, null, null, null); } public static RollbackTableRequest rollbackTableRequestByTag(String tagName) { - return new RollbackTableRequest(Instant.tag(tagName), null, null, null); + return new RollbackTableRequest(Instant.tag(tagName), null, null, null, null); } public static AlterViewRequest alterViewRequest() { diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogServer.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogServer.java index b4afe6adaff4..98449676be11 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogServer.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogServer.java @@ -727,12 +727,14 @@ && isTableByIdRequest(request.getPath())) { if (!tableMetadataStore.containsKey(identifier.getFullName())) { throw new Catalog.TableNotExistException(identifier); } + MockRESTMessage.validateRollbackHead( + requestBody, + tableLatestSnapshotStore.get(identifier.getFullName())); if (requestBody.getInstant() instanceof Instant.SnapshotInstant) { long snapshotId = ((Instant.SnapshotInstant) requestBody.getInstant()) .getSnapshotId(); - return rollbackTableByIdHandle( - identifier, snapshotId, requestBody.getFromSnapshot()); + return rollbackTableByIdHandle(identifier, snapshotId); } else if (requestBody.getInstant() instanceof Instant.TagInstant) { String tagName = ((Instant.TagInstant) requestBody.getInstant()) @@ -1195,8 +1197,8 @@ private MockResponse commitTableHandle(Identifier identifier, String data) throw requestBody.getStatistics()); } - private MockResponse rollbackTableByIdHandle( - Identifier identifier, long snapshotId, @Nullable Long fromSnapshot) throws Exception { + private MockResponse rollbackTableByIdHandle(Identifier identifier, long snapshotId) + throws Exception { FileStoreTable table = getFileTable(identifier); String identifierWithSnapshotId = geTableFullNameWithSnapshotId(identifier, snapshotId); TableSnapshot toSnapshot = tableWithSnapshotId2SnapshotStore.get(identifierWithSnapshotId); @@ -1207,16 +1209,6 @@ private MockResponse rollbackTableByIdHandle( 404); } long latestSnapshotId = table.snapshotManager().latestSnapshotId(); - if (fromSnapshot != null && fromSnapshot != latestSnapshotId) { - return mockResponse( - new ErrorResponse( - null, - null, - String.format( - "Latest snapshot %s is not %s", latestSnapshotId, fromSnapshot), - 500), - 500); - } table = table.copy( Collections.singletonMap(SNAPSHOT_CLEAN_EMPTY_DIRECTORIES.key(), "true")); diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogTest.java index 67106e1616a1..9a5e1af6ed8a 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogTest.java @@ -2378,9 +2378,22 @@ public void testTableRollback() throws Exception { // rollback to snapshot 2 from snapshot assertThatThrownBy( - () -> catalog.rollbackTo(identifier, null, Instant.snapshot(2L), 4L, null)) + () -> + catalog.rollbackTo( + identifier, + null, + Instant.snapshot(2L), + 3L, + "stale-head-uuid", + "writer")) + .hasMessageContaining("Latest snapshot UUID changed"); + assertThat(table.latestSnapshot().get().id()).isEqualTo(3); + assertThatThrownBy( + () -> + catalog.rollbackTo( + identifier, null, Instant.snapshot(2L), 4L, null, null)) .hasMessageContaining("Latest snapshot 3 is not 4"); - catalog.rollbackTo(identifier, null, Instant.snapshot(2L), 3L, null); + catalog.rollbackTo(identifier, null, Instant.snapshot(2L), 3L, null, null); assertThat(table.latestSnapshot().get().id()).isEqualTo(2); } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java index 56f1a8624828..e6a236283f05 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCommitLockIntegrationTest.java @@ -325,9 +325,9 @@ public MockResponse dispatch(RecordedRequest request) { assertThat(requestBody.getTableId()).isEqualTo("branch-id"); assertThat(requestBody.getFromSnapshot()) .isEqualTo(head.get().id()); - assertThat(requestBody.getCommitUser()).isEqualTo(owner.get()); if (locking) { assertThat(owner.get()).isNotNull(); + assertThat(requestBody.getCommitUser()).isEqualTo(owner.get()); } head.set( snapshots.get( @@ -424,10 +424,10 @@ public MockResponse dispatch(RecordedRequest request) { assertThat(replacement.uuid()).isNotEqualTo(compactHead.uuid()); assertThat(replacement.commitKind()).isEqualTo(Snapshot.CommitKind.OVERWRITE); assertThat(rollback.get().getFromSnapshot()).isEqualTo(6L); + assertThat(rollback.get().getFromSnapshotUuid()).isEqualTo(compactHead.uuid()); assertThat(((Instant.SnapshotInstant) rollback.get().getInstant()).getSnapshotId()) .isEqualTo(5L); - assertThat(rollback.get().getCommitUser()) - .isEqualTo(locking ? replacement.commitUser() : null); + assertThat(rollback.get().getCommitUser()).isEqualTo(replacement.commitUser()); if (locking) { assertThat(events) .containsSubsequence( diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RollbackProtocolTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RollbackProtocolTest.java index b26aad689960..4e710c0f1cf9 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RollbackProtocolTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RollbackProtocolTest.java @@ -72,7 +72,7 @@ public CatalogLoader catalogLoader() { .setBody("{}")); String tableUuid = "without-uuid".equals(context) ? null : "table-id"; String commitUser = "owned".equals(context) ? "Writer-1" : null; - catalog.rollbackTo(identifier, tableUuid, instant, 2L, commitUser); + catalog.rollbackTo(identifier, tableUuid, instant, 2L, "head-uuid", commitUser); RecordedRequest recorded = server.takeRequest(10, TimeUnit.SECONDS); assertThat(recorded.getMethod()).isEqualTo("POST"); assertThat(recorded.getPath()) @@ -82,6 +82,7 @@ public CatalogLoader catalogLoader() { assertThat(payload.get("instant").get("type").asText()).isEqualTo(kind); RollbackTableRequest request = RESTApi.fromJson(body, RollbackTableRequest.class); assertThat(request.getFromSnapshot()).isEqualTo(2L); + assertThat(request.getFromSnapshotUuid()).isEqualTo("head-uuid"); if ("snapshot".equals(kind)) { assertThat(((Instant.SnapshotInstant) request.getInstant()).getSnapshotId()) .isEqualTo(1L); @@ -100,7 +101,7 @@ public CatalogLoader catalogLoader() { assertThat(payload.has("commitUser")).isFalse(); } assertThat(payload.size()) - .isEqualTo(2 + (tableUuid == null ? 0 : 1) + (commitUser == null ? 0 : 1)); + .isEqualTo(3 + (tableUuid == null ? 0 : 1) + (commitUser == null ? 0 : 1)); } } } diff --git a/paimon-core/src/test/java/org/apache/paimon/table/CatalogEnvironmentTest.java b/paimon-core/src/test/java/org/apache/paimon/table/CatalogEnvironmentTest.java index 1b18b7039b4a..3489b8e50604 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/CatalogEnvironmentTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/CatalogEnvironmentTest.java @@ -80,10 +80,15 @@ void testCatalogTableRollbackUsesRuntimeBranchAndOwner(String branch) throws Exc true, false); Instant instant = Instant.snapshot(1L); - environment.catalogTableRollback(branch, "writer").rollbackTo(instant, 2L); + environment.catalogTableRollback(branch, "writer").rollbackTo(instant, 2L, "head-uuid"); verify(catalog) .rollbackTo( - new Identifier("db", "table", branch), "table-id", instant, 2L, "writer"); + new Identifier("db", "table", branch), + "table-id", + instant, + 2L, + "head-uuid", + "writer"); } @ParameterizedTest @@ -102,9 +107,15 @@ void testCatalogTableRollbackWithoutOwnerPreservesTableUuid(String branch, Strin true, false); Instant instant = Instant.tag("tag-1"); - environment.catalogTableRollback(null, null).rollbackTo(instant, null); + environment.catalogTableRollback(null, null).rollbackTo(instant, null, null); verify(catalog) - .rollbackTo(new Identifier("db", "table", branch), tableUuid, instant, null, null); + .rollbackTo( + new Identifier("db", "table", branch), + tableUuid, + instant, + null, + null, + null); } @ParameterizedTest @@ -131,12 +142,12 @@ void testCopiedEnvironmentRollbackUsesTargetIdentity( CatalogEnvironment copied = environment.copy(targetIdentifier); Instant snapshot = Instant.snapshot(1L); - copied.catalogTableRollback(null, null).rollbackTo(snapshot, 2L); - verify(catalog).rollbackTo(targetIdentifier, expectedUuid, snapshot, 2L, null); + copied.catalogTableRollback(null, null).rollbackTo(snapshot, 2L, "head-uuid"); + verify(catalog).rollbackTo(targetIdentifier, expectedUuid, snapshot, 2L, "head-uuid", null); Instant tag = Instant.tag("checkpoint"); copied.snapshotLoader().rollback(tag); - verify(catalog).rollbackTo(targetIdentifier, expectedUuid, tag, null, null); + verify(catalog).rollbackTo(targetIdentifier, expectedUuid, tag, null, null, null); assertThat(environment.uuid()).isEqualTo(sourceUuid); } diff --git a/paimon-core/src/test/java/org/apache/paimon/table/FallbackReadFileStoreTableTest.java b/paimon-core/src/test/java/org/apache/paimon/table/FallbackReadFileStoreTableTest.java index 78ac14e2683c..213de33561d1 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/FallbackReadFileStoreTableTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/FallbackReadFileStoreTableTest.java @@ -579,6 +579,7 @@ void testBranchViewsRollbackWithoutSourceUuid(String branchOption) throws Except .getSnapshotId() == 1L), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull()); table.switchToBranch("bc").rollbackTo("checkpoint"); @@ -594,6 +595,7 @@ void testBranchViewsRollbackWithoutSourceUuid(String branchOption) throws Except ((Instant.TagInstant) instant) .getTagName())), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull()); assertThat(table.catalogEnvironment().uuid()).isEqualTo("main-uuid"); } diff --git a/paimon-core/src/test/java/org/apache/paimon/tag/SnapshotLoaderImplTest.java b/paimon-core/src/test/java/org/apache/paimon/tag/SnapshotLoaderImplTest.java index 772564281b18..0346690bad54 100644 --- a/paimon-core/src/test/java/org/apache/paimon/tag/SnapshotLoaderImplTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/tag/SnapshotLoaderImplTest.java @@ -43,7 +43,7 @@ void testRollbackPreservesTableUuid(String branch, String uuid) throws Exception environment.snapshotLoader().rollback(instant); - verify(catalog).rollbackTo(identifier, uuid, instant, null, null); + verify(catalog).rollbackTo(identifier, uuid, instant, null, null, null); } @ParameterizedTest @@ -62,7 +62,12 @@ void testCopyWithinBranchKeepsTableUuid(String branch) throws Exception { verify(catalog) .rollbackTo( - new Identifier("db", "table", branch), "table-uuid", instant, null, null); + new Identifier("db", "table", branch), + "table-uuid", + instant, + null, + null, + null); } @ParameterizedTest @@ -78,7 +83,7 @@ void testCopyToAnotherBranchDoesNotReuseSourceUuid(String source, String target) loader.copyWithBranch(target).rollback(instant); verify(catalog) - .rollbackTo(new Identifier("db", "table", target), null, instant, null, null); + .rollbackTo(new Identifier("db", "table", target), null, instant, null, null, null); } private CatalogEnvironment environment(Catalog catalog, Identifier identifier, String uuid) { From bfa282c176e6cd4e1c9e117b992ac2e110dffcb3 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Tue, 13 Oct 2026 15:25:50 +0800 Subject: [PATCH 8/9] [rest] Retire idle commit lease timer threads --- docs/docs/concepts/concurrency-control.md | 8 +- .../apache/paimon/rest/RESTCatalogLock.java | 4 +- .../paimon/rest/RESTCatalogLockTest.java | 99 +++++++++++++++++++ 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/docs/docs/concepts/concurrency-control.md b/docs/docs/concepts/concurrency-control.md index 6bc8f8dc3cf3..b3d641a4c4ac 100644 --- a/docs/docs/concepts/concurrency-control.md +++ b/docs/docs/concepts/concurrency-control.md @@ -118,9 +118,11 @@ Lock contention waits until `commit.timeout` when configured, independently of p Scopes share a timer and a cached worker pool. The timer dispatches RPCs to workers so a blocked renewal cannot block other scopes' scheduling. Each scope permits only one in-flight renewal and cancels its own scheduled and in-flight renewal tasks when it ends, without stopping the shared -executors. The client validates the lease's monotonic deadline before publishing. A failed operation -stops renewal and its lease expires automatically. The server keeps the lease duration fixed for each -scope; duration changes take effect on the next acquisition. +executors. Timer and worker threads exit after one minute of inactivity and restart when needed. +The timer stays alive while renewal tasks remain scheduled. The client validates the lease's +monotonic deadline before publishing. A failed operation stops renewal and its lease expires +automatically. The server keeps the lease duration fixed for each scope; duration changes take effect +on the next acquisition. The server must validate the current table incarnation, branch, authenticated caller and exact owner, and atomically publish the snapshot and clear the lease. All snapshot writers must honor diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java index 1f902f3f35fb..3e896982a3da 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java @@ -102,11 +102,13 @@ public RESTCatalogLock( this.nanoTime = nanoTime; } - private static ScheduledExecutorService createRenewalScheduler() { + static ScheduledThreadPoolExecutor createRenewalScheduler() { ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor( 1, new ExecutorThreadFactory("rest-commit-lock-timer")); executor.setRemoveOnCancelPolicy(true); + executor.setKeepAliveTime(1, TimeUnit.MINUTES); + executor.allowCoreThreadTimeOut(true); return executor; } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java index 7c9181618d42..5472817c9959 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java @@ -40,7 +40,9 @@ import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import static org.assertj.core.api.Assertions.assertThat; @@ -71,6 +73,103 @@ class RESTCatalogLockTest { new RESTCatalogLock( api, identifier, "table-id", "morax-job", 0, renewer, workers, clock::get); + @Test + void idleSchedulerCanRestartAfterTheLastLeaseEnds() throws Exception { + grant("morax-job", 60000); + ScheduledThreadPoolExecutor scheduler = RESTCatalogLock.createRenewalScheduler(); + scheduler.setKeepAliveTime(10, TimeUnit.MILLISECONDS); + RESTCatalogLock realLock = + new RESTCatalogLock( + api, + identifier, + "table-id", + "morax-job", + 0, + scheduler, + workers, + clock::get); + try { + Thread firstTimer; + try (AutoCloseable ignored = realLock.acquire()) { + firstTimer = scheduler.submit(Thread::currentThread).get(10, TimeUnit.SECONDS); + realLock.ensureValid(); + } + firstTimer.join(2000); + assertThat(firstTimer.isAlive()).isFalse(); + assertThat(scheduler.getPoolSize()).isZero(); + + Thread nextTimer; + try (AutoCloseable ignored = realLock.acquire()) { + nextTimer = scheduler.submit(Thread::currentThread).get(10, TimeUnit.SECONDS); + assertThat(nextTimer).isNotSameAs(firstTimer); + realLock.ensureValid(); + } + nextTimer.join(2000); + assertThat(nextTimer.isAlive()).isFalse(); + } finally { + scheduler.shutdownNow(); + assertThat(scheduler.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void closingOneLeaseKeepsTheSharedSchedulerRenewingOtherLeases() throws Exception { + grant("morax-job", 3000); + CommitLockResponse otherGrant = + new CommitLockResponse(true, "other-job", 100000, 3000, null); + when(api.acquireCommitLock(identifier, "table-id", "other-job")).thenReturn(otherGrant); + CountDownLatch otherRenewed = new CountDownLatch(1); + AtomicBoolean firstClosed = new AtomicBoolean(); + when(api.renewCommitLock(identifier, "table-id", "other-job")) + .thenAnswer( + invocation -> { + if (firstClosed.get()) { + otherRenewed.countDown(); + } + return otherGrant; + }); + ScheduledThreadPoolExecutor scheduler = RESTCatalogLock.createRenewalScheduler(); + scheduler.setKeepAliveTime(10, TimeUnit.MILLISECONDS); + RESTCatalogLock firstLock = + new RESTCatalogLock( + api, + identifier, + "table-id", + "morax-job", + 0, + scheduler, + workers, + clock::get); + RESTCatalogLock otherLock = + new RESTCatalogLock( + api, + identifier, + "table-id", + "other-job", + 0, + scheduler, + workers, + clock::get); + Thread timer; + try { + try (AutoCloseable first = firstLock.acquire(); + AutoCloseable other = otherLock.acquire()) { + timer = scheduler.submit(Thread::currentThread).get(10, TimeUnit.SECONDS); + first.close(); + firstClosed.set(true); + assertThat(otherRenewed.await(10, TimeUnit.SECONDS)).isTrue(); + otherLock.ensureValid(); + assertThat(timer.isAlive()).isTrue(); + } + timer.join(2000); + assertThat(timer.isAlive()).isFalse(); + assertThat(scheduler.getQueue()).isEmpty(); + } finally { + scheduler.shutdownNow(); + assertThat(scheduler.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + @Test void callbackIsProtectedAndLeaseEndsOnReturn() throws Exception { grant("morax-job", 60000); From 566a0a8cf6cbde738262afe030e080d6f38ffa58 Mon Sep 17 00:00:00 2001 From: JingsongLi Date: Tue, 13 Oct 2026 16:46:24 +0800 Subject: [PATCH 9/9] [rest] Clarify worker-side lease validation --- .../apache/paimon/rest/RESTCatalogLock.java | 9 ++-- .../paimon/rest/RESTCatalogLockTest.java | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java index 3e896982a3da..581881048462 100644 --- a/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java +++ b/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalogLock.java @@ -123,7 +123,7 @@ public AutoCloseable acquire() throws Exception { validateGrant(response); Lease lease = new Lease(response.getLeaseMillis(), requestedAt); try { - lease.ensureValid(); + lease.checkValid(); currentLease.set(lease); return () -> { if (currentLease.get() == lease) { @@ -169,7 +169,7 @@ public void ensureValid() { if (lease == null) { throw new IllegalStateException("No commit lease is held by this thread."); } - lease.ensureValid(); + lease.checkValid(); } private class Lease implements AutoCloseable { @@ -226,7 +226,8 @@ private void renew() { return; } try { - ensureValid(); + // Renewal workers validate this lease without the acquiring thread's context. + checkValid(); long requestedAt = nanoTime.getAsLong(); CommitLockResponse response = api.renewCommitLock(identifier, tableUuid, commitUser); @@ -244,7 +245,7 @@ private void renew() { } } - public void ensureValid() { + private void checkValid() { // Include response time conservatively without relying on the renewal worker running. if (TimeUnit.NANOSECONDS.toMillis(nanoTime.getAsLong() - renewedAt) >= leaseMillis && renewalFailure == null) { diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java index 5472817c9959..91856529bba3 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogLockTest.java @@ -198,6 +198,50 @@ void callbackFailureStopsRenewal() { verify(future).cancel(false); } + @Test + void renewalOnAnotherThreadExtendsTheAcquiringThreadsLease() throws Exception { + grant("morax-job", 60000); + ExecutorService renewalWorker = Executors.newSingleThreadExecutor(); + RESTCatalogLock realLock = + new RESTCatalogLock( + api, + identifier, + "table-id", + "morax-job", + 0, + renewer, + renewalWorker, + clock::get); + Thread acquiringThread = Thread.currentThread(); + when(api.renewCommitLock(identifier, "table-id", "morax-job")) + .thenAnswer( + invocation -> { + assertThat(Thread.currentThread()).isNotSameAs(acquiringThread); + assertThatThrownBy(realLock::ensureValid) + .hasMessageContaining("No commit lease"); + return new CommitLockResponse(true, "morax-job", 140000, 60000, null); + }); + try { + try (AutoCloseable ignored = realLock.acquire()) { + clock.set(TimeUnit.MILLISECONDS.toNanos(40000)); + renewal().run(); + // A FIFO barrier waits until the renewal and its deadline update have completed. + renewalWorker.submit(() -> {}).get(10, TimeUnit.SECONDS); + verify(api).renewCommitLock(identifier, "table-id", "morax-job"); + + clock.set(TimeUnit.MILLISECONDS.toNanos(70000)); + realLock.ensureValid(); + clock.set(TimeUnit.MILLISECONDS.toNanos(100000)); + assertThatThrownBy(realLock::ensureValid) + .hasRootCauseMessage("The commit lease has expired locally."); + } + verify(future).cancel(false); + } finally { + renewalWorker.shutdownNow(); + assertThat(renewalWorker.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + @Test void renewalUsesExactOwnerAndStopsAfterClose() throws Exception { grant("morax-job", 60000);