You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2026/07/17 21:45:50 UTC
[PR] Replace transaction metastore wrapper with explicit buffering [polaris]
flyrain opened a new pull request, #5088:
URL: https://github.com/apache/polaris/pull/5088
## What changed
- Replaced `TransactionWorkspaceMetaStoreManager` with a package-private `PendingTableCommit` collector scoped to Iceberg table commits.
- Kept the real `PolarisMetaStoreManager` immutable and made transaction buffering explicit at the entity-update and metadata-cleanup call sites.
- Preserved the existing rejection behavior for location-overlap validation during a pending multi-table commit.
- Added coverage for buffered entity updates, deferred metadata cleanup, and the preserved overlap-validation failure.
## Why
`TransactionWorkspaceMetaStoreManager` implemented the entire metastore interface even though it only buffered one write operation. Nearly every other method threw, and its delegate was unused. This made a catalog-specific transaction detail look like a general persistence implementation and coupled it to every change in `PolarisMetaStoreManager`.
The new collector represents only the side effects that `commitTransaction` actually defers: table-entity updates and metadata-file cleanup.
## Impact
This is an internal refactor with no intended user-visible behavior change. Multi-table commits still persist table entity updates atomically, defer removed-metadata cleanup until persistence succeeds, and reject location-overlap validation in the same cases as before.
Transaction-aware validation against pending table locations is intentionally left for a follow-up PR.
## Validation
- `./gradlew format compileAll`
- `./gradlew :polaris-core:check`
- Runtime service Spotless and Checkstyle
- Focused transaction, cleanup, event, table-location, and overlap tests across default, NoSQL, and relational persistence profiles
- `./gradlew check` was also attempted. It reached `:polaris-runtime-service:intTest` and failed because local integration test resources did not initialize: Keycloak returned HTTP 403, and MinIO/RustFS access objects were not injected. Focused tests and module checks for this change passed.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: no related issue
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [x] ๐งพ Updated `CHANGELOG.md` (not needed; no intended user-visible behavior change)
- [x] ๐ Updated documentation in `site/content/in-dev/unreleased` (not needed; no API or configuration change)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3616511452
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1699,7 +1690,13 @@ public void commit(TableMetadata base, TableMetadata metadata) {
long start = System.currentTimeMillis();
doCommit(base, metadata);
- deleteRemovedMetadataFiles.accept(new MetadataFileCleanup(io(), base, metadata));
+ MetadataFileCleanup cleanup = new MetadataFileCleanup(io(), base, metadata);
+ if (pendingTableCommit != null) {
+ pendingTableCommit.addPendingMetadataFileCleanup(cleanup);
Review Comment:
Made this change by introducing a new interface and two impl. for different behaviors. I'm OK with both. I think the first version is actually a bit simpler. Let me know if you strongly prefer one over another.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3616213874
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1699,7 +1690,13 @@ public void commit(TableMetadata base, TableMetadata metadata) {
long start = System.currentTimeMillis();
doCommit(base, metadata);
- deleteRemovedMetadataFiles.accept(new MetadataFileCleanup(io(), base, metadata));
+ MetadataFileCleanup cleanup = new MetadataFileCleanup(io(), base, metadata);
+ if (pendingTableCommit != null) {
+ pendingTableCommit.addPendingMetadataFileCleanup(cleanup);
Review Comment:
Hi @dimas-b, that's a good point. The conditional block isn't that elegant. What you suggest also makes sense. That's actually my first version. But I intentionally switch it to back to this version due to at least one or two additional classes/interfaces added. I thought maybe this is maybe simpler, given there are only two places needing it. And we can always abstract later by introducing the interface, etc. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3609056292
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1699,7 +1690,13 @@ public void commit(TableMetadata base, TableMetadata metadata) {
long start = System.currentTimeMillis();
doCommit(base, metadata);
- deleteRemovedMetadataFiles.accept(new MetadataFileCleanup(io(), base, metadata));
+ MetadataFileCleanup cleanup = new MetadataFileCleanup(io(), base, metadata);
+ if (pendingTableCommit != null) {
+ pendingTableCommit.addPendingMetadataFileCleanup(cleanup);
Review Comment:
This and similar (new) if/else block complicate the processing logic. How about renaming `PendingTableCommit` to `CatalogEntityUpdater` and making two implementations:
1) calling through to `metaStoreManager` for single entity changes.
2) collecting updates and metadata cleanup for multi-table commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5088: Replace transaction metastore wrapper with explicit buffering
URL: https://github.com/apache/polaris/pull/5088
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3616624161
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -201,20 +200,14 @@ record MetadataWriteResult(String location, boolean written) {}
private Map<String, String> catalogProperties;
private final StorageAccessConfigProvider storageAccessConfigProvider;
private final FileIOFactory fileIOFactory;
- private PolarisMetaStoreManager metaStoreManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final TableCommitActions directTableCommitActions;
Review Comment:
nit: it's not always "direct" ๐ค
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5088:
URL: https://github.com/apache/polaris/pull/5088#issuecomment-5350264241
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5088:
URL: https://github.com/apache/polaris/pull/5088#issuecomment-6244363817
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3609056292
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1699,7 +1690,13 @@ public void commit(TableMetadata base, TableMetadata metadata) {
long start = System.currentTimeMillis();
doCommit(base, metadata);
- deleteRemovedMetadataFiles.accept(new MetadataFileCleanup(io(), base, metadata));
+ MetadataFileCleanup cleanup = new MetadataFileCleanup(io(), base, metadata);
+ if (pendingTableCommit != null) {
+ pendingTableCommit.addPendingMetadataFileCleanup(cleanup);
Review Comment:
This and similar (new) if/else blocks complicate the processing logic. How about renaming `PendingTableCommit` to `CatalogEntityUpdater` and making two implementations:
1) calling through to `metaStoreManager` for single entity changes.
2) collecting updates and metadata cleanup for multi-table commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3606450247
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1443,16 +1442,10 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest)
baseCatalog.getClass().getName());
}
- // Swap in TransactionWorkspaceMetaStoreManager for all mutations made by this baseCatalog to
- // only go into an in-memory collection that we can commit as a single atomic unit after all
- // validations.
- TransactionWorkspaceMetaStoreManager transactionMetaStoreManager =
- new TransactionWorkspaceMetaStoreManager(diagnostics(), metaStoreManager());
-
- List<MetadataFileCleanup> pendingCleanups = new ArrayList<>();
-
- ((LocalIcebergCatalog) baseCatalog)
- .setMetaStoreManager(transactionMetaStoreManager, pendingCleanups::add);
+ // Buffer the entity updates and metadata-file cleanups produced by each table commit. The
+ // entity updates are persisted as a single atomic unit after all validations succeed.
+ PendingTableCommit pendingTableCommit = new PendingTableCommit();
+ ((LocalIcebergCatalog) baseCatalog).setPendingTableCommit(pendingTableCommit);
Review Comment:
commitTransaction() sets a PendingTableCommit on the shared LocalIcebergCatalog instance but never clears it. If this catalog instance is reused after commitTransaction returns (or if any exception is thrown before normal return, including during metadata cleanup), subsequent catalog operations will incorrectly buffer entity updates / metadata cleanups and may start throwing "illegal_method_in_transaction_workspace" from location-overlap validation.
Please ensure the pending-table-commit state is always cleared (success and failure) by wrapping the body of commitTransaction in a try/finally and calling ((LocalIcebergCatalog) baseCatalog).resetPendingTableCommit() in the finally block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace transaction metastore wrapper with explicit buffering [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5088:
URL: https://github.com/apache/polaris/pull/5088#discussion_r3616694582
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java:
##########
@@ -256,6 +257,33 @@ protected LocalIcebergCatalog newIcebergCatalog(
eventMetadataFactory);
}
+ @Test
+ public void testBufferedTableCommitActionsPreserveOptimizedLocationOverlapFailure() {
Review Comment:
I'm not sure what this test validates? Does it mean that `IcebergCatalogHandler.commitTransaction()` _never_ called this method?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -274,6 +267,8 @@ public LocalIcebergCatalog(
this.storageAccessConfigProvider = storageAccessConfigProvider;
this.fileIOFactory = fileIOFactory;
this.metaStoreManager = metaStoreManager;
+ this.directTableCommitActions = new DirectTableCommitActions(metaStoreManager);
Review Comment:
A fresh `LocalIcebergCatalog` is created for each REST API call, including `IcebergCatalogHandler.commitTransaction()`.
Ideally, the type of `tableCommitActions` to use should trickle down through the call chain as it is specific to overarching operation as a whole.
If you agree, this can be deferred to keep this PR's diff smaller.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1440,6 +1429,10 @@ private <T extends PolarisEntity & LocationBasedEntity> void validateNoLocationO
// Attempt to directly query for siblings
boolean useOptimizedSiblingCheck =
realmConfig.getConfig(FeatureConfiguration.OPTIMIZED_SIBLING_CHECK);
+ if (tableCommitActions instanceof BufferedTableCommitActions) {
+ String methodName = useOptimizedSiblingCheck ? "hasOverlappingSiblings" : "listEntities";
+ throw diagnostics.fail("illegal_method_in_transaction_workspace", methodName);
Review Comment:
This logic looks odd in the current codebase ๐ค Why do we need this?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/BufferedTableCommitActions.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.polaris.service.catalog.iceberg;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.persistence.dao.entity.EntityResult;
+import org.apache.polaris.core.persistence.dao.entity.EntityWithPath;
+import org.jspecify.annotations.NonNull;
+
+/** Buffers table-commit actions until a multi-table transaction can apply them atomically. */
+final class BufferedTableCommitActions implements TableCommitActions {
+ private final List<EntityWithPath> pendingUpdates = new ArrayList<>();
+ private final List<MetadataFileCleanup> pendingMetadataFileCleanups = new ArrayList<>();
+
+ @Override
+ public EntityResult updateEntityPropertiesIfNotChanged(
+ @NonNull PolarisCallContext callContext,
+ @NonNull List<PolarisEntityCore> catalogPath,
+ @NonNull PolarisBaseEntity entity) {
+ pendingUpdates.add(new EntityWithPath(catalogPath, entity));
+ return new EntityResult(entity);
+ }
+
+ @Override
+ public void cleanupMetadataFiles(@NonNull MetadataFileCleanup cleanup) {
+ pendingMetadataFileCleanups.add(cleanup);
+ }
+
+ List<EntityWithPath> pendingUpdates() {
+ return List.copyOf(pendingUpdates);
Review Comment:
also maybe move `updateEntitiesPropertiesIfNotChanged()` calls and matching "clean up" call here too?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/BufferedTableCommitActions.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.polaris.service.catalog.iceberg;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.persistence.dao.entity.EntityResult;
+import org.apache.polaris.core.persistence.dao.entity.EntityWithPath;
+import org.jspecify.annotations.NonNull;
+
+/** Buffers table-commit actions until a multi-table transaction can apply them atomically. */
+final class BufferedTableCommitActions implements TableCommitActions {
+ private final List<EntityWithPath> pendingUpdates = new ArrayList<>();
+ private final List<MetadataFileCleanup> pendingMetadataFileCleanups = new ArrayList<>();
+
+ @Override
+ public EntityResult updateEntityPropertiesIfNotChanged(
+ @NonNull PolarisCallContext callContext,
+ @NonNull List<PolarisEntityCore> catalogPath,
+ @NonNull PolarisBaseEntity entity) {
+ pendingUpdates.add(new EntityWithPath(catalogPath, entity));
+ return new EntityResult(entity);
+ }
+
+ @Override
+ public void cleanupMetadataFiles(@NonNull MetadataFileCleanup cleanup) {
+ pendingMetadataFileCleanups.add(cleanup);
+ }
+
+ List<EntityWithPath> pendingUpdates() {
+ return List.copyOf(pendingUpdates);
+ }
+
+ List<MetadataFileCleanup> pendingMetadataFileCleanups() {
+ return List.copyOf(pendingMetadataFileCleanups);
Review Comment:
Perhaps we should move the batched call to `CatalogUtil.deleteRemovedMetadataFiles()` here to be symmetric with `DirectTableCommitActions`? WDYT?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -201,20 +200,14 @@ record MetadataWriteResult(String location, boolean written) {}
private Map<String, String> catalogProperties;
private final StorageAccessConfigProvider storageAccessConfigProvider;
private final FileIOFactory fileIOFactory;
- private PolarisMetaStoreManager metaStoreManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final TableCommitActions directTableCommitActions;
Review Comment:
nit: it's not always "direct" ๐ค
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -330,20 +325,14 @@ public void initialize(String name, Map<String, String> properties) {
PropertyUtil.propertiesWithPrefix(properties, CatalogProperties.TABLE_DEFAULT_PREFIX);
}
- public void setMetaStoreManager(PolarisMetaStoreManager newMetaStoreManager) {
- setMetaStoreManager(newMetaStoreManager, null);
+ /** Uses the supplied actions for table-entity updates and metadata-file cleanup. */
+ void setTableCommitActions(@NonNull TableCommitActions tableCommitActions) {
+ this.tableCommitActions = tableCommitActions;
}
- /**
- * Sets the meta store manager and optionally a custom logic for deleting removed metadata files.
- * This allows the caller (e.g. during commitTransaction) to provide a collector instead of
- * immediate deletion, avoiding premature deletes that could lead to corruption on rollback.
- */
- public void setMetaStoreManager(
- PolarisMetaStoreManager newMetaStoreManager, Consumer<MetadataFileCleanup> deleteLogic) {
- this.metaStoreManager = newMetaStoreManager;
- this.deleteRemovedMetadataFiles =
- deleteLogic != null ? deleteLogic : LocalIcebergCatalog::defaultDeleteRemovedMetadataFiles;
+ /** Restores immediate persistence and metadata-file cleanup for table commits. */
+ void resetTableCommitActions() {
Review Comment:
I think this method is rather confusing. The local catalog object should not be shared between operations that have different "table commit" semantics.
Currently it's called only from test code, but those tests also pass without this method, I believe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org