From 6c90a3d6dfaec73f2e979c8be0a1d4169b3c6c10 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 11:45:26 -0800 Subject: [PATCH 01/40] wip --- .../polaris/service/catalog/BasePolarisCatalog.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 3d926fb00..9e9ed37af 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -811,6 +811,17 @@ public Map getCredentialConfig( storageInfo.get()); } + @Override + public Table loadTable(TableIdentifier identifier) { + // #### + boolean allowMetadata = callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), + PolarisConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES); + } + /** * Based on configuration settings, for callsites that need to handle potentially setting a new * base location for a TableLike entity, produces the transformed location if applicable, or else From bda008ee95cd8003b739f23bf71b5d564c56f8c0 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 16:13:54 -0800 Subject: [PATCH 02/40] first real commit --- .../polaris/core/PolarisConfiguration.java | 8 ++ .../core/entity/PolarisEntityType.java | 3 +- .../core/entity/TableMetadataEntity.java | 68 +++++++++++++ .../PolarisMetaStoreManagerImpl.java | 3 + .../service/catalog/BasePolarisCatalog.java | 28 +++++- .../persistence/MetadataCacheManager.java | 95 +++++++++++++++++++ 6 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java create mode 100644 polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index 397c9afd9..6266cbcbf 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -191,4 +191,12 @@ public static Builder builder() { "If set to true, allows tables to be dropped with the purge parameter set to true.") .defaultValue(true) .build(); + + public static final PolarisConfiguration USE_METADATA_CACHE = + PolarisConfiguration.builder() + .key("USE_METADATA_CACHE") + .description( + "If set to true, support serving table metadata without reading the metadata.json file.") + .defaultValue(false) + .build(); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java index fa3a2d661..308dcb818 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java @@ -34,7 +34,8 @@ public enum PolarisEntityType { // generic table is either a view or a real table TABLE_LIKE(7, NAMESPACE, false, false), TASK(8, ROOT, false, false), - FILE(9, TABLE_LIKE, false, false); + FILE(9, TABLE_LIKE, false, false), + TABLE_METADATA(10, TABLE_LIKE, false, false); // to efficiently map a code to its corresponding entity type, use a reverse array which // is initialized below diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java new file mode 100644 index 000000000..6afd4f118 --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -0,0 +1,68 @@ +/* + * 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.core.entity; + +import com.fasterxml.jackson.annotation.JsonIgnore; + +public class TableMetadataEntity extends PolarisEntity { + private static String CONTENT_KEY = "content"; + private static String METADATA_LOCATION_KEY = "metadata_location"; + + public TableMetadataEntity(PolarisBaseEntity sourceEntity) { + super(sourceEntity); + } + + public static TableMetadataEntity of(PolarisBaseEntity sourceEntity) { + if (sourceEntity != null) { + return new TableMetadataEntity(sourceEntity); + } + return null; + } + + @JsonIgnore + public String getContent() { + return getInternalPropertiesAsMap().get(CONTENT_KEY); + } + + @JsonIgnore + public String getMetadataLocation() { + return getInternalPropertiesAsMap().get(METADATA_LOCATION_KEY); + } + + public static class Builder extends PolarisEntity.BaseBuilder { + public Builder(String metadataLocation, String content) { + this.internalProperties.put(CONTENT_KEY, content); + } + + @Override + public TableMetadataEntity build() { + return new TableMetadataEntity(buildBase()); + } + + public TableMetadataEntity.Builder setContent(String content) { + this.internalProperties.put(CONTENT_KEY, content); + return this; + } + + public TableMetadataEntity.Builder setMetadataLocation(String metadataLocation) { + this.internalProperties.put(METADATA_LOCATION_KEY, metadataLocation); + return this; + } + } +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java index b05129fa3..2095ab33b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -49,6 +50,8 @@ import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; +import org.apache.polaris.core.entity.TableMetadataEntity; +import org.apache.polaris.core.persistence.models.ModelEntityActive; import org.apache.polaris.core.storage.PolarisCredentialProperty; import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 9e9ed37af..8dc2358c8 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -37,6 +37,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.commons.lang3.exception.ExceptionUtils; @@ -99,6 +100,7 @@ import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; import org.apache.polaris.core.storage.PolarisStorageIntegration; import org.apache.polaris.core.storage.aws.PolarisS3FileIOClientFactory; +import org.apache.polaris.service.persistence.MetadataCacheManager; import org.apache.polaris.service.task.TaskExecutor; import org.apache.polaris.service.types.NotificationRequest; import org.apache.polaris.service.types.NotificationType; @@ -811,15 +813,31 @@ public Map getCredentialConfig( storageInfo.get()); } - @Override - public Table loadTable(TableIdentifier identifier) { - // #### - boolean allowMetadata = callContext + public TableMetadata loadTableMetadata(TableIdentifier identifier) { + boolean useMetadataCache = callContext .getPolarisCallContext() .getConfigurationStore() .getConfiguration( callContext.getPolarisCallContext(), - PolarisConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES); + PolarisConfiguration.USE_METADATA_CACHE); + if (!useMetadataCache) { + return loadTableMetadata(loadTable(identifier)); + } else { + Supplier fallback = () -> loadTableMetadata(loadTable(identifier)); + return MetadataCacheManager.loadTableMetadata( + identifier, + callContext.getPolarisCallContext(), + entityManager, + resolvedEntityView, + fallback); + } + } + + private static TableMetadata loadTableMetadata(Table table) { + if (table instanceof BaseTable baseTable) { + return baseTable.operations().current(); + } + throw new IllegalArgumentException("Cannot load metadata for " + table.name()); } /** diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java new file mode 100644 index 000000000..5aaa35f24 --- /dev/null +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -0,0 +1,95 @@ +/* + * 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.persistence; + +import org.apache.iceberg.Table; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.TableMetadataParser; +import org.apache.iceberg.TableOperations; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.hadoop.HadoopTableOperations; +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.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.entity.TableLikeEntity; +import org.apache.polaris.core.entity.TableMetadataEntity; +import org.apache.polaris.core.persistence.PolarisEntityManager; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; +import org.jetbrains.annotations.NotNull; + +import java.util.List; +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + +public class MetadataCacheManager { + + /** + * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist + */ + public static TableMetadata loadTableMetadata( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView, + Supplier fallback) { + // TODO add write to cache + return loadCachedTableMetadata( + tableIdentifier, callContext, entityManager, resolvedEntityView) + .orElseGet(fallback); + } + + /** + * Return the cached {@link Table} entity, if one exists + */ + private static @NotNull Optional loadCachedTableMetadata( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath( + tableIdentifier, PolarisEntitySubType.TABLE); + if (resolvedEntities == null) { + return Optional.empty(); + } else { + TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); + String metadataLocation = entity.getMetadataLocation(); + PolarisMetaStoreManager.EntityResult metadataEntityResult = entityManager + .getMetaStoreManager() + .readEntityByName( + callContext, + resolvedEntities.getRawFullPath().stream().map(PolarisEntityCore::new).toList(), + PolarisEntityType.TABLE_METADATA, + PolarisEntitySubType.ANY_SUBTYPE, + metadataLocation); + return Optional + .ofNullable(metadataEntityResult.getEntity()) + .map(metadataEntity -> { + TableMetadataEntity tableMetadataEntity = (TableMetadataEntity) metadataEntity; + return TableMetadataParser.fromJson(tableMetadataEntity.getContent()); + }); + } + } +} From c38a01df46d46049a492ac39682a93b1fd7846cc Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 16:38:02 -0800 Subject: [PATCH 03/40] cleaning up --- .../persistence/MetadataCacheManager.java | 61 +++++++++++++++++-- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 5aaa35f24..ea2aa1061 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -27,6 +27,7 @@ import org.apache.iceberg.hadoop.HadoopTableOperations; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.PolarisBaseEntity; +import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntityCore; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; @@ -36,7 +37,10 @@ import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; +import org.apache.polaris.service.auth.TestOAuth2ApiService; import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.List; import java.util.Optional; @@ -44,6 +48,7 @@ import java.util.function.Supplier; public class MetadataCacheManager { + private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); /** * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist @@ -54,10 +59,56 @@ public static TableMetadata loadTableMetadata( PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView, Supplier fallback) { - // TODO add write to cache - return loadCachedTableMetadata( - tableIdentifier, callContext, entityManager, resolvedEntityView) - .orElseGet(fallback); + LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); + Optional cachedMetadata = loadCachedTableMetadata( + tableIdentifier, callContext, entityManager, resolvedEntityView); + if (cachedMetadata.isPresent()) { + LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); + return cachedMetadata.get(); + } else { + TableMetadata metadata = fallback.get(); + PolarisMetaStoreManager.EntityResult cacheResult = cacheTableMetadata( + tableIdentifier, + metadata, + callContext, + entityManager, + resolvedEntityView); + if (!cacheResult.isSuccess()) { + LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); + } + return metadata; + } + } + + /** + * Attempt to add table metadata to the cache + * @return The result of trying to cache the metadata + */ + private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( + TableIdentifier tableIdentifier, + TableMetadata metadata, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath( + tableIdentifier, PolarisEntitySubType.TABLE); + if (resolvedEntities == null) { + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.ENTITY_NOT_FOUND, null); + } else { + TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); + TableMetadataEntity tableMetadataEntity = + new TableMetadataEntity.Builder(metadata.location(), TableMetadataParser.toJson(metadata)) + .setParentId(tableEntity.getId()) + .build(); + return entityManager + .getMetaStoreManager() + .createEntityIfNotExists( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + tableMetadataEntity); + } } /** @@ -80,7 +131,7 @@ public static TableMetadata loadTableMetadata( .getMetaStoreManager() .readEntityByName( callContext, - resolvedEntities.getRawFullPath().stream().map(PolarisEntityCore::new).toList(), + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), PolarisEntityType.TABLE_METADATA, PolarisEntitySubType.ANY_SUBTYPE, metadataLocation); From 3a699531029c1fdc4a4f8b533ef8a483d673c89e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 16:46:19 -0800 Subject: [PATCH 04/40] tests fixed --- .../org/apache/polaris/core/entity/TableMetadataEntity.java | 6 +++++- .../polaris/service/persistence/MetadataCacheManager.java | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java index 6afd4f118..13aa28ca4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -47,7 +47,11 @@ public String getMetadataLocation() { public static class Builder extends PolarisEntity.BaseBuilder { public Builder(String metadataLocation, String content) { - this.internalProperties.put(CONTENT_KEY, content); + super(); + setName(metadataLocation); + setType(PolarisEntityType.TABLE_METADATA); + setMetadataLocation(metadataLocation); + setContent(content); } @Override diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index ea2aa1061..0692d6cbf 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -100,7 +100,9 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); TableMetadataEntity tableMetadataEntity = new TableMetadataEntity.Builder(metadata.location(), TableMetadataParser.toJson(metadata)) + .setCatalogId(tableEntity.getCatalogId()) .setParentId(tableEntity.getId()) + .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) .build(); return entityManager .getMetaStoreManager() From ca856edb49e9a0ad695507ebc2cb8248bf095963 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 16:50:32 -0800 Subject: [PATCH 05/40] begin refactoring --- .../catalog/PolarisCatalogHandlerWrapper.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 29aa328c7..26b361df9 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -822,10 +822,21 @@ public LoadTableResponse loadTableWithAccessDelegation( // when data-access is specified but access delegation grants are not found. return doCatalogOperation( () -> { - Table table = baseCatalog.loadTable(tableIdentifier); + final TableMetadata tableMetadata; + if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { + tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); + } else { + // TODO fix + Table table = baseCatalog.loadTable(tableIdentifier); + if (table instanceof BaseTable baseTable) { + tableMetadata = baseTable.operations().current(); + } else { + tableMetadata = null; + } + } + if (table instanceof BaseTable baseTable) { - TableMetadata tableMetadata = baseTable.operations().current(); LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(tableMetadata); if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { From 779d9122d5fee43379085e1f5270e4c7a5d06871 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 17:02:49 -0800 Subject: [PATCH 06/40] wire up to loadTable --- .../catalog/PolarisCatalogHandlerWrapper.java | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 26b361df9..2c0a9eb30 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -792,7 +792,15 @@ public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snaps PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); - return doCatalogOperation(() -> CatalogHandlers.loadTable(baseCatalog, tableIdentifier)); + return doCatalogOperation(() -> { + if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { + return LoadTableResponse.builder() + .withTableMetadata(basePolarisCatalog.loadTableMetadata(tableIdentifier)) + .build(); + } + + return CatalogHandlers.loadTable(baseCatalog, tableIdentifier); + }); } public LoadTableResponse loadTableWithAccessDelegation( @@ -822,40 +830,31 @@ public LoadTableResponse loadTableWithAccessDelegation( // when data-access is specified but access delegation grants are not found. return doCatalogOperation( () -> { - final TableMetadata tableMetadata; + TableMetadata tableMetadata = null; if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); - } else { - // TODO fix - Table table = baseCatalog.loadTable(tableIdentifier); - if (table instanceof BaseTable baseTable) { - tableMetadata = baseTable.operations().current(); - } else { - tableMetadata = null; - } } + // The metadata failed to load + if (tableMetadata == null) { + throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); + } - if (table instanceof BaseTable baseTable) { + if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(tableMetadata); - if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { - LOGGER - .atDebug() - .addKeyValue("tableIdentifier", tableIdentifier) - .addKeyValue("tableLocation", tableMetadata.location()) - .log("Fetching client credentials for table"); - responseBuilder.addAllConfig( - credentialDelegation.getCredentialConfig( - tableIdentifier, tableMetadata, actionsRequested)); - } + LOGGER + .atDebug() + .addKeyValue("tableIdentifier", tableIdentifier) + .addKeyValue("tableLocation", tableMetadata.location()) + .log("Fetching client credentials for table"); + responseBuilder.addAllConfig( + credentialDelegation.getCredentialConfig( + tableIdentifier, tableMetadata, actionsRequested)); return responseBuilder.build(); - } else if (table instanceof BaseMetadataTable) { - // metadata tables are loaded on the client side, return NoSuchTableException for now - throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); + } else { + throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); } - - throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); }); } From b4c533546cb83a5f43b4446c34203a2aeb3ca67d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 20:49:15 -0800 Subject: [PATCH 07/40] autolint --- .../core/entity/TableMetadataEntity.java | 77 +++---- .../PolarisMetaStoreManagerImpl.java | 3 - .../service/catalog/BasePolarisCatalog.java | 12 +- .../catalog/PolarisCatalogHandlerWrapper.java | 17 +- .../persistence/MetadataCacheManager.java | 188 ++++++++---------- 5 files changed, 140 insertions(+), 157 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java index 13aa28ca4..ae12ded44 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -21,52 +21,53 @@ import com.fasterxml.jackson.annotation.JsonIgnore; public class TableMetadataEntity extends PolarisEntity { - private static String CONTENT_KEY = "content"; - private static String METADATA_LOCATION_KEY = "metadata_location"; + private static String CONTENT_KEY = "content"; + private static String METADATA_LOCATION_KEY = "metadata_location"; - public TableMetadataEntity(PolarisBaseEntity sourceEntity) { - super(sourceEntity); - } + public TableMetadataEntity(PolarisBaseEntity sourceEntity) { + super(sourceEntity); + } - public static TableMetadataEntity of(PolarisBaseEntity sourceEntity) { - if (sourceEntity != null) { - return new TableMetadataEntity(sourceEntity); - } - return null; + public static TableMetadataEntity of(PolarisBaseEntity sourceEntity) { + if (sourceEntity != null) { + return new TableMetadataEntity(sourceEntity); } + return null; + } - @JsonIgnore - public String getContent() { - return getInternalPropertiesAsMap().get(CONTENT_KEY); - } + @JsonIgnore + public String getContent() { + return getInternalPropertiesAsMap().get(CONTENT_KEY); + } - @JsonIgnore - public String getMetadataLocation() { - return getInternalPropertiesAsMap().get(METADATA_LOCATION_KEY); - } + @JsonIgnore + public String getMetadataLocation() { + return getInternalPropertiesAsMap().get(METADATA_LOCATION_KEY); + } - public static class Builder extends PolarisEntity.BaseBuilder { - public Builder(String metadataLocation, String content) { - super(); - setName(metadataLocation); - setType(PolarisEntityType.TABLE_METADATA); - setMetadataLocation(metadataLocation); - setContent(content); - } + public static class Builder + extends PolarisEntity.BaseBuilder { + public Builder(String metadataLocation, String content) { + super(); + setName(metadataLocation); + setType(PolarisEntityType.TABLE_METADATA); + setMetadataLocation(metadataLocation); + setContent(content); + } - @Override - public TableMetadataEntity build() { - return new TableMetadataEntity(buildBase()); - } + @Override + public TableMetadataEntity build() { + return new TableMetadataEntity(buildBase()); + } - public TableMetadataEntity.Builder setContent(String content) { - this.internalProperties.put(CONTENT_KEY, content); - return this; - } + public TableMetadataEntity.Builder setContent(String content) { + this.internalProperties.put(CONTENT_KEY, content); + return this; + } - public TableMetadataEntity.Builder setMetadataLocation(String metadataLocation) { - this.internalProperties.put(METADATA_LOCATION_KEY, metadataLocation); - return this; - } + public TableMetadataEntity.Builder setMetadataLocation(String metadataLocation) { + this.internalProperties.put(METADATA_LOCATION_KEY, metadataLocation); + return this; } + } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java index 2095ab33b..b05129fa3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java @@ -30,7 +30,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -50,8 +49,6 @@ import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; -import org.apache.polaris.core.entity.TableMetadataEntity; -import org.apache.polaris.core.persistence.models.ModelEntityActive; import org.apache.polaris.core.storage.PolarisCredentialProperty; import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 8dc2358c8..c2462a11e 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -814,12 +814,12 @@ public Map getCredentialConfig( } public TableMetadata loadTableMetadata(TableIdentifier identifier) { - boolean useMetadataCache = callContext - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - callContext.getPolarisCallContext(), - PolarisConfiguration.USE_METADATA_CACHE); + boolean useMetadataCache = + callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), PolarisConfiguration.USE_METADATA_CACHE); if (!useMetadataCache) { return loadTableMetadata(loadTable(identifier)); } else { diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 2c0a9eb30..6e41168c5 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -792,15 +792,16 @@ public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snaps PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); - return doCatalogOperation(() -> { - if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { - return LoadTableResponse.builder() - .withTableMetadata(basePolarisCatalog.loadTableMetadata(tableIdentifier)) - .build(); - } + return doCatalogOperation( + () -> { + if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { + return LoadTableResponse.builder() + .withTableMetadata(basePolarisCatalog.loadTableMetadata(tableIdentifier)) + .build(); + } - return CatalogHandlers.loadTable(baseCatalog, tableIdentifier); - }); + return CatalogHandlers.loadTable(baseCatalog, tableIdentifier); + }); } public LoadTableResponse loadTableWithAccessDelegation( diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 0692d6cbf..c94b80a23 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -16,19 +16,16 @@ * specific language governing permissions and limitations * under the License. */ - package org.apache.polaris.service.persistence; +import java.util.Optional; +import java.util.function.Supplier; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableMetadataParser; -import org.apache.iceberg.TableOperations; import org.apache.iceberg.catalog.TableIdentifier; -import org.apache.iceberg.hadoop.HadoopTableOperations; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; -import org.apache.polaris.core.entity.PolarisEntityCore; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.TableLikeEntity; @@ -37,112 +34,99 @@ import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; -import org.apache.polaris.service.auth.TestOAuth2ApiService; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.List; -import java.util.Optional; -import java.util.function.Function; -import java.util.function.Supplier; - public class MetadataCacheManager { - private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); + private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); - /** - * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist - */ - public static TableMetadata loadTableMetadata( - TableIdentifier tableIdentifier, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView, - Supplier fallback) { - LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); - Optional cachedMetadata = loadCachedTableMetadata( - tableIdentifier, callContext, entityManager, resolvedEntityView); - if (cachedMetadata.isPresent()) { - LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); - return cachedMetadata.get(); - } else { - TableMetadata metadata = fallback.get(); - PolarisMetaStoreManager.EntityResult cacheResult = cacheTableMetadata( - tableIdentifier, - metadata, - callContext, - entityManager, - resolvedEntityView); - if (!cacheResult.isSuccess()) { - LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); - } - return metadata; - } + /** Load the cached {@link Table} or fall back to `fallback` if one doesn't exist */ + public static TableMetadata loadTableMetadata( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView, + Supplier fallback) { + LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); + Optional cachedMetadata = + loadCachedTableMetadata(tableIdentifier, callContext, entityManager, resolvedEntityView); + if (cachedMetadata.isPresent()) { + LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); + return cachedMetadata.get(); + } else { + TableMetadata metadata = fallback.get(); + PolarisMetaStoreManager.EntityResult cacheResult = + cacheTableMetadata( + tableIdentifier, metadata, callContext, entityManager, resolvedEntityView); + if (!cacheResult.isSuccess()) { + LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); + } + return metadata; } + } - /** - * Attempt to add table metadata to the cache - * @return The result of trying to cache the metadata - */ - private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( - TableIdentifier tableIdentifier, - TableMetadata metadata, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath( - tableIdentifier, PolarisEntitySubType.TABLE); - if (resolvedEntities == null) { - return new PolarisMetaStoreManager.EntityResult( - PolarisMetaStoreManager.ReturnStatus.ENTITY_NOT_FOUND, null); - } else { - TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - TableMetadataEntity tableMetadataEntity = - new TableMetadataEntity.Builder(metadata.location(), TableMetadataParser.toJson(metadata)) - .setCatalogId(tableEntity.getCatalogId()) - .setParentId(tableEntity.getId()) - .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) - .build(); - return entityManager - .getMetaStoreManager() - .createEntityIfNotExists( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - tableMetadataEntity); - } + /** + * Attempt to add table metadata to the cache + * + * @return The result of trying to cache the metadata + */ + private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( + TableIdentifier tableIdentifier, + TableMetadata metadata, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); + if (resolvedEntities == null) { + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.ENTITY_NOT_FOUND, null); + } else { + TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); + TableMetadataEntity tableMetadataEntity = + new TableMetadataEntity.Builder(metadata.location(), TableMetadataParser.toJson(metadata)) + .setCatalogId(tableEntity.getCatalogId()) + .setParentId(tableEntity.getId()) + .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) + .build(); + return entityManager + .getMetaStoreManager() + .createEntityIfNotExists( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + tableMetadataEntity); } + } - /** - * Return the cached {@link Table} entity, if one exists - */ - private static @NotNull Optional loadCachedTableMetadata( - TableIdentifier tableIdentifier, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath( - tableIdentifier, PolarisEntitySubType.TABLE); - if (resolvedEntities == null) { - return Optional.empty(); - } else { - TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - String metadataLocation = entity.getMetadataLocation(); - PolarisMetaStoreManager.EntityResult metadataEntityResult = entityManager - .getMetaStoreManager() - .readEntityByName( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - PolarisEntityType.TABLE_METADATA, - PolarisEntitySubType.ANY_SUBTYPE, - metadataLocation); - return Optional - .ofNullable(metadataEntityResult.getEntity()) - .map(metadataEntity -> { - TableMetadataEntity tableMetadataEntity = (TableMetadataEntity) metadataEntity; - return TableMetadataParser.fromJson(tableMetadataEntity.getContent()); - }); - } + /** Return the cached {@link Table} entity, if one exists */ + private static @NotNull Optional loadCachedTableMetadata( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); + if (resolvedEntities == null) { + return Optional.empty(); + } else { + TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); + String metadataLocation = entity.getMetadataLocation(); + PolarisMetaStoreManager.EntityResult metadataEntityResult = + entityManager + .getMetaStoreManager() + .readEntityByName( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + PolarisEntityType.TABLE_METADATA, + PolarisEntitySubType.ANY_SUBTYPE, + metadataLocation); + return Optional.ofNullable(metadataEntityResult.getEntity()) + .map( + metadataEntity -> { + TableMetadataEntity tableMetadataEntity = (TableMetadataEntity) metadataEntity; + return TableMetadataParser.fromJson(tableMetadataEntity.getContent()); + }); } + } } From 6e23be40d114d445b0e792c9f4b29d7b1aa94002 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 21:41:06 -0800 Subject: [PATCH 08/40] logic to purge old records --- .../core/entity/TableMetadataEntity.java | 8 ++- .../persistence/MetadataCacheManager.java | 49 ++++++++++++++----- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java index ae12ded44..93a1bbdd7 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -20,9 +20,13 @@ import com.fasterxml.jackson.annotation.JsonIgnore; +/** + * A {@link PolarisEntity} for storing table metadata. This can contain the raw content + * of the `metadata.json` or more granular information + */ public class TableMetadataEntity extends PolarisEntity { - private static String CONTENT_KEY = "content"; - private static String METADATA_LOCATION_KEY = "metadata_location"; + private static final String CONTENT_KEY = "content"; + private static final String METADATA_LOCATION_KEY = "metadata_location"; public TableMetadataEntity(PolarisBaseEntity sourceEntity) { super(sourceEntity); diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index c94b80a23..a235cd05b 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -18,6 +18,7 @@ */ package org.apache.polaris.service.persistence; +import java.util.Collection; import java.util.Optional; import java.util.function.Supplier; import org.apache.iceberg.Table; @@ -25,6 +26,7 @@ import org.apache.iceberg.TableMetadataParser; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; @@ -112,21 +114,42 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } else { TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); String metadataLocation = entity.getMetadataLocation(); - PolarisMetaStoreManager.EntityResult metadataEntityResult = - entityManager - .getMetaStoreManager() - .readEntityByName( + PolarisMetaStoreManager.ListEntitiesResult metadataResult = entityManager + .getMetaStoreManager() + .listEntities( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + PolarisEntityType.TABLE_METADATA, + PolarisEntitySubType.ANY_SUBTYPE + ); + return Optional.ofNullable(metadataResult.getEntities()) + .stream() + .flatMap(Collection::stream) + .flatMap(result -> { + PolarisMetaStoreManager.EntityResult metadataEntityResult = + entityManager.getMetaStoreManager() + .loadEntity(callContext, result.getCatalogId(), result.getId()); + return Optional.ofNullable(metadataEntityResult.getEntity()) + .map(e -> (TableMetadataEntity) e) + .stream(); + }) + .filter(metadata -> { + if (metadata.getMetadataLocation().equals(metadataLocation)) { + return true; // Keep this metadata as it's the one we're interested in + } else { + LOGGER.debug(String.format("Deleting old entry for %s", metadata.getMetadataLocation())); + entityManager.getMetaStoreManager().dropEntityIfExists( callContext, PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - PolarisEntityType.TABLE_METADATA, - PolarisEntitySubType.ANY_SUBTYPE, - metadataLocation); - return Optional.ofNullable(metadataEntityResult.getEntity()) - .map( - metadataEntity -> { - TableMetadataEntity tableMetadataEntity = (TableMetadataEntity) metadataEntity; - return TableMetadataParser.fromJson(tableMetadataEntity.getContent()); - }); + metadata, + null, + /*purge=*/ false + ); + return false; + } + }) + .findFirst() + .map(metadataEntity -> TableMetadataParser.fromJson(metadataEntity.getContent())); } } } From 5805679240855ae2d34e32b79688c77aa3aa32fa Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 21:41:13 -0800 Subject: [PATCH 09/40] autolint --- .../core/entity/TableMetadataEntity.java | 4 +- .../persistence/MetadataCacheManager.java | 69 ++++++++++--------- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java index 93a1bbdd7..0add5a1e9 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -21,8 +21,8 @@ import com.fasterxml.jackson.annotation.JsonIgnore; /** - * A {@link PolarisEntity} for storing table metadata. This can contain the raw content - * of the `metadata.json` or more granular information + * A {@link PolarisEntity} for storing table metadata. This can contain the raw content of the + * `metadata.json` or more granular information */ public class TableMetadataEntity extends PolarisEntity { private static final String CONTENT_KEY = "content"; diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index a235cd05b..e28d97cd6 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -26,7 +26,6 @@ import org.apache.iceberg.TableMetadataParser; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; @@ -114,40 +113,44 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } else { TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); String metadataLocation = entity.getMetadataLocation(); - PolarisMetaStoreManager.ListEntitiesResult metadataResult = entityManager - .getMetaStoreManager() - .listEntities( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - PolarisEntityType.TABLE_METADATA, - PolarisEntitySubType.ANY_SUBTYPE - ); - return Optional.ofNullable(metadataResult.getEntities()) - .stream() - .flatMap(Collection::stream) - .flatMap(result -> { - PolarisMetaStoreManager.EntityResult metadataEntityResult = - entityManager.getMetaStoreManager() - .loadEntity(callContext, result.getCatalogId(), result.getId()); - return Optional.ofNullable(metadataEntityResult.getEntity()) - .map(e -> (TableMetadataEntity) e) - .stream(); - }) - .filter(metadata -> { - if (metadata.getMetadataLocation().equals(metadataLocation)) { - return true; // Keep this metadata as it's the one we're interested in - } else { - LOGGER.debug(String.format("Deleting old entry for %s", metadata.getMetadataLocation())); - entityManager.getMetaStoreManager().dropEntityIfExists( + PolarisMetaStoreManager.ListEntitiesResult metadataResult = + entityManager + .getMetaStoreManager() + .listEntities( callContext, PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - metadata, - null, - /*purge=*/ false - ); - return false; - } - }) + PolarisEntityType.TABLE_METADATA, + PolarisEntitySubType.ANY_SUBTYPE); + return Optional.ofNullable(metadataResult.getEntities()).stream() + .flatMap(Collection::stream) + .flatMap( + result -> { + PolarisMetaStoreManager.EntityResult metadataEntityResult = + entityManager + .getMetaStoreManager() + .loadEntity(callContext, result.getCatalogId(), result.getId()); + return Optional.ofNullable(metadataEntityResult.getEntity()) + .map(e -> (TableMetadataEntity) e) + .stream(); + }) + .filter( + metadata -> { + if (metadata.getMetadataLocation().equals(metadataLocation)) { + return true; // Keep this metadata as it's the one we're interested in + } else { + LOGGER.debug( + String.format("Deleting old entry for %s", metadata.getMetadataLocation())); + entityManager + .getMetaStoreManager() + .dropEntityIfExists( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + metadata, + null, + /* purge= */ false); + return false; + } + }) .findFirst() .map(metadataEntity -> TableMetadataParser.fromJson(metadataEntity.getContent())); } From f32a82c773f8e25095825259b79bb8cfa17d4901 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 21:48:23 -0800 Subject: [PATCH 10/40] stable, disabled --- .../java/org/apache/polaris/core/PolarisConfiguration.java | 4 ++-- .../apache/polaris/service/catalog/BasePolarisCatalog.java | 2 +- .../polaris/service/persistence/MetadataCacheManager.java | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index 6266cbcbf..a0109a752 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -192,9 +192,9 @@ public static Builder builder() { .defaultValue(true) .build(); - public static final PolarisConfiguration USE_METADATA_CACHE = + public static final PolarisConfiguration METADATA_CACHE_ENABLED = PolarisConfiguration.builder() - .key("USE_METADATA_CACHE") + .key("METADATA_CACHE_ENABLED") .description( "If set to true, support serving table metadata without reading the metadata.json file.") .defaultValue(false) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index c2462a11e..2100ebfa1 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -819,7 +819,7 @@ public TableMetadata loadTableMetadata(TableIdentifier identifier) { .getPolarisCallContext() .getConfigurationStore() .getConfiguration( - callContext.getPolarisCallContext(), PolarisConfiguration.USE_METADATA_CACHE); + callContext.getPolarisCallContext(), PolarisConfiguration.METADATA_CACHE_ENABLED); if (!useMetadataCache) { return loadTableMetadata(loadTable(identifier)); } else { diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index e28d97cd6..178417d7b 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -90,6 +90,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( .setCatalogId(tableEntity.getCatalogId()) .setParentId(tableEntity.getId()) .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) + .setCreateTimestamp(System.currentTimeMillis()) .build(); return entityManager .getMetaStoreManager() From 249ecac33d191414dd9cd27e1f041920fe5c03be Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 6 Nov 2024 23:06:49 -0800 Subject: [PATCH 11/40] passing when enabled --- .../main/java/org/apache/polaris/core/PolarisConfiguration.java | 2 +- .../polaris/service/persistence/MetadataCacheManager.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index a0109a752..4d9e4e292 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -197,6 +197,6 @@ public static Builder builder() { .key("METADATA_CACHE_ENABLED") .description( "If set to true, support serving table metadata without reading the metadata.json file.") - .defaultValue(false) + .defaultValue(true) .build(); } diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 178417d7b..d9c16ba3f 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -131,7 +131,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( .getMetaStoreManager() .loadEntity(callContext, result.getCatalogId(), result.getId()); return Optional.ofNullable(metadataEntityResult.getEntity()) - .map(e -> (TableMetadataEntity) e) + .map(TableMetadataEntity::of) .stream(); }) .filter( From 0db85eb527a1422839d269a21c0759bac6315b53 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 10:09:05 -0800 Subject: [PATCH 12/40] stable --- .../core/entity/TableMetadataEntity.java | 6 +- .../core/persistence/EntityCacheTest.java | 2 +- .../persistence/MetadataCacheManager.java | 6 +- .../catalog/BasePolarisCatalogTest.java | 88 ++++++++++++++++++- 4 files changed, 94 insertions(+), 8 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java index 0add5a1e9..ac9d089f9 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java @@ -51,12 +51,9 @@ public String getMetadataLocation() { public static class Builder extends PolarisEntity.BaseBuilder { - public Builder(String metadataLocation, String content) { + public Builder() { super(); - setName(metadataLocation); setType(PolarisEntityType.TABLE_METADATA); - setMetadataLocation(metadataLocation); - setContent(content); } @Override @@ -71,6 +68,7 @@ public TableMetadataEntity.Builder setContent(String content) { public TableMetadataEntity.Builder setMetadataLocation(String metadataLocation) { this.internalProperties.put(METADATA_LOCATION_KEY, metadataLocation); + this.setName(metadataLocation); return this; } } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/persistence/EntityCacheTest.java b/polaris-core/src/test/java/org/apache/polaris/core/persistence/EntityCacheTest.java index 01062f335..b1033022f 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/persistence/EntityCacheTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/persistence/EntityCacheTest.java @@ -86,7 +86,7 @@ public EntityCacheTest() { callCtx = new PolarisCallContext(metaStore, diagServices); metaStoreManager = new PolarisMetaStoreManagerImpl(); - // bootstrap the mata store with our test schema + // bootstrap the metastore with our test schema tm = new PolarisTestMetaStoreManager(metaStoreManager, callCtx); tm.testCreateTestCatalog(); } diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index d9c16ba3f..392459400 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -86,11 +86,13 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } else { TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); TableMetadataEntity tableMetadataEntity = - new TableMetadataEntity.Builder(metadata.location(), TableMetadataParser.toJson(metadata)) + new TableMetadataEntity.Builder() .setCatalogId(tableEntity.getCatalogId()) .setParentId(tableEntity.getId()) .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) .setCreateTimestamp(System.currentTimeMillis()) + .setMetadataLocation(metadata.metadataFileLocation()) + .setContent(TableMetadataParser.toJson(metadata)) .build(); return entityManager .getMetaStoreManager() @@ -137,7 +139,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( .filter( metadata -> { if (metadata.getMetadataLocation().equals(metadataLocation)) { - return true; // Keep this metadata as it's the one we're interested in + return true; } else { LOGGER.debug( String.format("Deleting old entry for %s", metadata.getMetadataLocation())); diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index 63f859b76..4551aea97 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -25,8 +25,10 @@ import com.google.common.collect.ImmutableMap; import java.io.IOException; +import java.nio.file.Path; import java.time.Clock; import java.util.Arrays; +import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.List; @@ -43,6 +45,7 @@ import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableMetadataParser; +import org.apache.iceberg.TableOperations; import org.apache.iceberg.catalog.CatalogTests; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.SupportsNamespaces; @@ -77,6 +80,7 @@ import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisMetaStoreSession; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; import org.apache.polaris.core.storage.PolarisCredentialProperty; import org.apache.polaris.core.storage.PolarisStorageIntegration; import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider; @@ -85,6 +89,7 @@ import org.apache.polaris.core.storage.cache.StorageCredentialCache; import org.apache.polaris.service.admin.PolarisAdminService; import org.apache.polaris.service.persistence.InMemoryPolarisMetaStoreManagerFactory; +import org.apache.polaris.service.persistence.MetadataCacheManager; import org.apache.polaris.service.task.TableCleanupTaskHandler; import org.apache.polaris.service.task.TaskExecutor; import org.apache.polaris.service.task.TaskFileIOSupplier; @@ -98,6 +103,7 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.mockito.Mockito; import software.amazon.awssdk.services.sts.StsClient; import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; @@ -125,6 +131,7 @@ public class BasePolarisCatalogTest extends CatalogTests { private PolarisEntityManager entityManager; private AuthenticatedPolarisPrincipal authenticatedRoot; private PolarisEntity catalogEntity; + private PolarisResolutionManifestCatalogView passthroughView; @BeforeEach @SuppressWarnings("unchecked") @@ -200,7 +207,7 @@ public void before() { .setStorageConfigurationInfo(storageConfigModel, storageLocation) .build()); - PolarisPassthroughResolutionView passthroughView = + passthroughView = new PolarisPassthroughResolutionView( callContext, entityManager, authenticatedRoot, CATALOG_NAME); TaskExecutor taskExecutor = Mockito.mock(); @@ -1384,4 +1391,83 @@ public void testFileIOWrapper() { .getFirst())); Assertions.assertThat(measured.getNumDeletedFiles()).as("A table was deleted").isGreaterThan(0); } + + private Schema buildSchema(int fields) { + Types.NestedField[] fieldsArray = new Types.NestedField[fields]; + for (int i = 0; i < fields; i++) { + fieldsArray[i] = Types.NestedField.optional(i, "field_" + i, Types.IntegerType.get()); + } + return new Schema(fieldsArray); + } + + private TableMetadata createTableMetadata(String location, Schema schema) { + return TableMetadata + .newTableMetadata( + schema, + PartitionSpec.unpartitioned(), + location, + Collections.emptyMap() + ); + } + + @Test + public void testMetadataCachingWithManualFallback() { + Namespace namespace = Namespace.of("manual-namespace"); + TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "t1"); + + Schema schema = buildSchema(10); + + catalog.createNamespace(namespace); + Table createdTable = catalog.createTable(tableIdentifier, schema); + TableMetadata originalMetadata = ((BaseTable)createdTable).operations().current(); + + TableMetadata loadedMetadata = MetadataCacheManager + .loadTableMetadata( + tableIdentifier, + polarisContext, + entityManager, + passthroughView, + () -> originalMetadata); + + // The first time, the fallback is called + Assertions.assertThat(loadedMetadata).isSameAs(originalMetadata); + + TableMetadata cachedMetadata = MetadataCacheManager + .loadTableMetadata( + tableIdentifier, + polarisContext, + entityManager, + passthroughView, + () -> { + throw new IllegalStateException("Fell back even though a cache entry should exist!"); + }); + + // The second time, it's loaded from the cache + Assertions.assertThat(cachedMetadata).isNotSameAs(originalMetadata); + + // The content should match what was cached + Assertions.assertThat(TableMetadataParser.toJson(cachedMetadata)) + .isEqualTo(TableMetadataParser.toJson(loadedMetadata)); + + // Update the table + TableOperations tableOps = catalog.newTableOps(tableIdentifier); + TableMetadata updatedMetadata = + tableOps.current().updateLocation(originalMetadata.location() + "-updated"); + tableOps.commit( + tableOps.current(), + updatedMetadata); + TableMetadata spyUpdatedMetadata = Mockito.spy(updatedMetadata); + Mockito.doReturn("spy-location").when(spyUpdatedMetadata).metadataFileLocation(); + + // Read from the cache; it should detect a chance due to the update and load the new fallback + TableMetadata reloadedMetadata = MetadataCacheManager + .loadTableMetadata( + tableIdentifier, + polarisContext, + entityManager, + passthroughView, + () -> spyUpdatedMetadata); + + Assertions.assertThat(reloadedMetadata).isSameAs(spyUpdatedMetadata); + } } From 6f0d9df3231274d59a3e33b4f02a36878040ff47 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 10:09:08 -0800 Subject: [PATCH 13/40] autolint --- .../catalog/BasePolarisCatalogTest.java | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index 4551aea97..524f07b0a 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -25,7 +25,6 @@ import com.google.common.collect.ImmutableMap; import java.io.IOException; -import java.nio.file.Path; import java.time.Clock; import java.util.Arrays; import java.util.Collections; @@ -103,7 +102,6 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; import org.mockito.Mockito; import software.amazon.awssdk.services.sts.StsClient; import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; @@ -1401,13 +1399,8 @@ private Schema buildSchema(int fields) { } private TableMetadata createTableMetadata(String location, Schema schema) { - return TableMetadata - .newTableMetadata( - schema, - PartitionSpec.unpartitioned(), - location, - Collections.emptyMap() - ); + return TableMetadata.newTableMetadata( + schema, PartitionSpec.unpartitioned(), location, Collections.emptyMap()); } @Test @@ -1419,10 +1412,10 @@ public void testMetadataCachingWithManualFallback() { catalog.createNamespace(namespace); Table createdTable = catalog.createTable(tableIdentifier, schema); - TableMetadata originalMetadata = ((BaseTable)createdTable).operations().current(); + TableMetadata originalMetadata = ((BaseTable) createdTable).operations().current(); - TableMetadata loadedMetadata = MetadataCacheManager - .loadTableMetadata( + TableMetadata loadedMetadata = + MetadataCacheManager.loadTableMetadata( tableIdentifier, polarisContext, entityManager, @@ -1432,8 +1425,8 @@ public void testMetadataCachingWithManualFallback() { // The first time, the fallback is called Assertions.assertThat(loadedMetadata).isSameAs(originalMetadata); - TableMetadata cachedMetadata = MetadataCacheManager - .loadTableMetadata( + TableMetadata cachedMetadata = + MetadataCacheManager.loadTableMetadata( tableIdentifier, polarisContext, entityManager, @@ -1453,15 +1446,13 @@ public void testMetadataCachingWithManualFallback() { TableOperations tableOps = catalog.newTableOps(tableIdentifier); TableMetadata updatedMetadata = tableOps.current().updateLocation(originalMetadata.location() + "-updated"); - tableOps.commit( - tableOps.current(), - updatedMetadata); + tableOps.commit(tableOps.current(), updatedMetadata); TableMetadata spyUpdatedMetadata = Mockito.spy(updatedMetadata); Mockito.doReturn("spy-location").when(spyUpdatedMetadata).metadataFileLocation(); // Read from the cache; it should detect a chance due to the update and load the new fallback - TableMetadata reloadedMetadata = MetadataCacheManager - .loadTableMetadata( + TableMetadata reloadedMetadata = + MetadataCacheManager.loadTableMetadata( tableIdentifier, polarisContext, entityManager, From f25091634ecf1964943d6ec4b50c4208fab41abc Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 11:05:03 -0800 Subject: [PATCH 14/40] autolint --- .../main/java/org/apache/polaris/core/PolarisConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index 21f574a40..115576748 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -197,7 +197,7 @@ public static Builder builder() { .catalogConfig("drop-with-purge.enabled") .description( "If set to true, allows tables to be dropped with the purge parameter set to true.") - .defaultValue(true) + .defaultValue(false) .build(); public static final PolarisConfiguration METADATA_CACHE_ENABLED = From dd87d423e086ff13731d449a0f5add31251cdbd5 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 11:05:48 -0800 Subject: [PATCH 15/40] oops --- .../java/org/apache/polaris/core/PolarisConfiguration.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index 115576748..a067cdd9f 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -197,7 +197,7 @@ public static Builder builder() { .catalogConfig("drop-with-purge.enabled") .description( "If set to true, allows tables to be dropped with the purge parameter set to true.") - .defaultValue(false) + .defaultValue(true) .build(); public static final PolarisConfiguration METADATA_CACHE_ENABLED = @@ -205,6 +205,6 @@ public static Builder builder() { .key("METADATA_CACHE_ENABLED") .description( "If set to true, support serving table metadata without reading the metadata.json file.") - .defaultValue(true) + .defaultValue(false) .build(); } From c46eb03e19997b37e97431fe46633cdd270c63c5 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 12:15:44 -0800 Subject: [PATCH 16/40] add try/catch --- .../persistence/MetadataCacheManager.java | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 392459400..642452da9 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -94,12 +94,25 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( .setMetadataLocation(metadata.metadataFileLocation()) .setContent(TableMetadataParser.toJson(metadata)) .build(); - return entityManager - .getMetaStoreManager() - .createEntityIfNotExists( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - tableMetadataEntity); + try { + return entityManager + .getMetaStoreManager() + .createEntityIfNotExists( + callContext, + PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), + tableMetadataEntity); + } catch (RuntimeException e) { + // PersistenceException (& other extension-specific exceptions) may not be in scope, + // but we can make a best-effort attempt to swallow it and just forego caching + if (e.toString().contains("PersistenceException")) { + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, + e.getMessage() + ); + } else { + throw e; + } + } } } From 1e1b953e1a95ced4db6492850f2c2e3713f1edc6 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 12:15:48 -0800 Subject: [PATCH 17/40] autolint --- .../polaris/service/persistence/MetadataCacheManager.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 642452da9..491249833 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -106,9 +106,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( // but we can make a best-effort attempt to swallow it and just forego caching if (e.toString().contains("PersistenceException")) { return new PolarisMetaStoreManager.EntityResult( - PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, - e.getMessage() - ); + PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, e.getMessage()); } else { throw e; } From a164807a04ddace6f91bb8cb683f9841d1f5d3f1 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 20:35:59 -0800 Subject: [PATCH 18/40] added drop-on-drop --- .../PolarisMetaStoreManagerImpl.java | 7 +++++ .../service/catalog/BasePolarisCatalog.java | 8 +++++ .../persistence/MetadataCacheManager.java | 30 ++++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java index b05129fa3..154c8dd1a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java @@ -24,15 +24,20 @@ import com.fasterxml.jackson.databind.ObjectMapper; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; + +import org.apache.iceberg.TableMetadata; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.AsyncTaskType; import org.apache.polaris.core.entity.PolarisBaseEntity; @@ -49,6 +54,8 @@ import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; +import org.apache.polaris.core.entity.TableLikeEntity; +import org.apache.polaris.core.entity.TableMetadataEntity; import org.apache.polaris.core.storage.PolarisCredentialProperty; import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index bdec582b5..b361dc523 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1890,6 +1890,14 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { } } + // Purge table metadata, if it exists: + MetadataCacheManager.dropTableMetadata( + identifier, + callContext.getPolarisCallContext(), + entityManager, + resolvedEntityView); + + // Drop the table: return getMetaStoreManager() .dropEntityIfExists( getCurrentPolarisContext(), diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 491249833..d0ef5da61 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -120,6 +120,34 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( PolarisCallContext callContext, PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView) { + return loadCachedTableMetadataImpl( + tableIdentifier, + callContext, + entityManager, + resolvedEntityView, + false); + } + + public static void dropTableMetadata( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView) { + loadCachedTableMetadataImpl( + tableIdentifier, + callContext, + entityManager, + resolvedEntityView, + true); + } + + /** Return the cached {@link Table} entity, if one exists */ + private static @NotNull Optional loadCachedTableMetadataImpl( + TableIdentifier tableIdentifier, + PolarisCallContext callContext, + PolarisEntityManager entityManager, + PolarisResolutionManifestCatalogView resolvedEntityView, + boolean dropEverything) { PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); if (resolvedEntities == null) { @@ -149,7 +177,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( }) .filter( metadata -> { - if (metadata.getMetadataLocation().equals(metadataLocation)) { + if (metadata.getMetadataLocation().equals(metadataLocation) && !dropEverything) { return true; } else { LOGGER.debug( From 6ed3f97f406fc6eb61152fd6070679819474fc32 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 20:36:03 -0800 Subject: [PATCH 19/40] autolint --- .../persistence/PolarisMetaStoreManagerImpl.java | 7 ------- .../polaris/service/catalog/BasePolarisCatalog.java | 5 +---- .../service/persistence/MetadataCacheManager.java | 12 ++---------- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java index 154c8dd1a..b05129fa3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java @@ -24,20 +24,15 @@ import com.fasterxml.jackson.databind.ObjectMapper; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; - -import org.apache.iceberg.TableMetadata; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.AsyncTaskType; import org.apache.polaris.core.entity.PolarisBaseEntity; @@ -54,8 +49,6 @@ import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; -import org.apache.polaris.core.entity.TableLikeEntity; -import org.apache.polaris.core.entity.TableMetadataEntity; import org.apache.polaris.core.storage.PolarisCredentialProperty; import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index b361dc523..596838b42 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1892,10 +1892,7 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { // Purge table metadata, if it exists: MetadataCacheManager.dropTableMetadata( - identifier, - callContext.getPolarisCallContext(), - entityManager, - resolvedEntityView); + identifier, callContext.getPolarisCallContext(), entityManager, resolvedEntityView); // Drop the table: return getMetaStoreManager() diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index d0ef5da61..c66ffac54 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -121,11 +121,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView) { return loadCachedTableMetadataImpl( - tableIdentifier, - callContext, - entityManager, - resolvedEntityView, - false); + tableIdentifier, callContext, entityManager, resolvedEntityView, false); } public static void dropTableMetadata( @@ -134,11 +130,7 @@ public static void dropTableMetadata( PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView) { loadCachedTableMetadataImpl( - tableIdentifier, - callContext, - entityManager, - resolvedEntityView, - true); + tableIdentifier, callContext, entityManager, resolvedEntityView, true); } /** Return the cached {@link Table} entity, if one exists */ From 35b852f1d71416ac11e275902ce8308bfc2ef023 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 17:30:10 -0800 Subject: [PATCH 20/40] refactor to remove new entity --- .../polaris/core/PolarisConfiguration.java | 46 ++++- .../core/entity/PolarisEntityType.java | 3 +- .../polaris/core/entity/TableLikeEntity.java | 14 ++ .../core/entity/TableMetadataEntity.java | 75 -------- .../service/catalog/BasePolarisCatalog.java | 11 +- .../persistence/MetadataCacheManager.java | 178 ++++++------------ .../catalog/BasePolarisCatalogTest.java | 15 +- 7 files changed, 130 insertions(+), 212 deletions(-) delete mode 100644 polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index a067cdd9f..86985f44e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -20,6 +20,8 @@ import java.util.List; import java.util.Optional; +import java.util.function.Function; + import org.apache.polaris.core.admin.model.StorageConfigInfo; public class PolarisConfiguration { @@ -29,15 +31,23 @@ public class PolarisConfiguration { public final T defaultValue; private final Optional catalogConfigImpl; private final Class typ; + private final Optional> validation; @SuppressWarnings("unchecked") public PolarisConfiguration( - String key, String description, T defaultValue, Optional catalogConfig) { + String key, + String description, + T defaultValue, + Optional catalogConfig, + Optional> validation) { this.key = key; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; this.typ = (Class) defaultValue.getClass(); + this.validation = validation; + + validate(cast(defaultValue)); } public boolean hasCatalogConfig() { @@ -52,7 +62,18 @@ public String catalogConfig() { } T cast(Object value) { - return this.typ.cast(value); + T result = this.typ.cast(value); + validate(result); + return result; + } + + private void validate(T value) { + this.validation.ifPresent(v -> { + if (!v.apply(value)) { + throw new IllegalArgumentException( + String.format("Configuration %s has invalid value %s", key, defaultValue)); + } + }); } public static class Builder { @@ -60,6 +81,7 @@ public static class Builder { private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); + private Optional> validation = Optional.empty(); public Builder key(String key) { this.key = key; @@ -81,11 +103,16 @@ public Builder catalogConfig(String catalogConfig) { return this; } + public Builder validation(Function validation) { + this.validation = Optional.of(validation); + return this; + } + public PolarisConfiguration build() { if (key == null || description == null || defaultValue == null) { throw new IllegalArgumentException("key, description, and defaultValue are required"); } - return new PolarisConfiguration<>(key, description, defaultValue, catalogConfig); + return new PolarisConfiguration<>(key, description, defaultValue, catalogConfig, validation); } } @@ -200,11 +227,14 @@ public static Builder builder() { .defaultValue(true) .build(); - public static final PolarisConfiguration METADATA_CACHE_ENABLED = - PolarisConfiguration.builder() - .key("METADATA_CACHE_ENABLED") + public static final Long METADATA_CACHE_MAX_BYTES_NO_CACHING = 0L; + public static final PolarisConfiguration METADATA_CACHE_MAX_BYTES = + PolarisConfiguration.builder() + .key("METADATA_CACHE_MAX_BYTES") .description( - "If set to true, support serving table metadata without reading the metadata.json file.") - .defaultValue(false) + "If nonzero, the max size a table's metadata can be in order to be cached in the persistence layer." + + " If zero, no metadata will be cached or served from the cache. If -1, all metadata will be cached.") + .defaultValue(METADATA_CACHE_MAX_BYTES_NO_CACHING) + .validation(value -> value >= -1) .build(); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java index 308dcb818..fa3a2d661 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java @@ -34,8 +34,7 @@ public enum PolarisEntityType { // generic table is either a view or a real table TABLE_LIKE(7, NAMESPACE, false, false), TASK(8, ROOT, false, false), - FILE(9, TABLE_LIKE, false, false), - TABLE_METADATA(10, TABLE_LIKE, false, false); + FILE(9, TABLE_LIKE, false, false); // to efficiently map a code to its corresponding entity type, use a reverse array which // is initialized below diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java index 968598b93..fb026c24c 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java @@ -29,6 +29,10 @@ public class TableLikeEntity extends PolarisEntity { // of the internalProperties JSON file. public static final String METADATA_LOCATION_KEY = "metadata-location"; + // For applicable types, this key on the "internalProperties" map will return the content of the + // metadata.json file located at `METADATA_LOCATION_KEY` + private static final String METADATA_CONTENT_KEY = "metadata-content"; + public static final String USER_SPECIFIED_WRITE_DATA_LOCATION_KEY = "write.data.path"; public static final String USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY = "write.metadata.path"; @@ -67,6 +71,11 @@ public String getMetadataLocation() { return getInternalPropertiesAsMap().get(METADATA_LOCATION_KEY); } + @JsonIgnore + public String getMetadataContent() { + return getInternalPropertiesAsMap().get(METADATA_CONTENT_KEY); + } + @JsonIgnore public Optional getLastAdmittedNotificationTimestamp() { return Optional.ofNullable( @@ -121,6 +130,11 @@ public Builder setMetadataLocation(String location) { return this; } + public Builder setMetadataContent(String content) { + internalProperties.put(METADATA_CONTENT_KEY, content); + return this; + } + public Builder setLastNotificationTimestamp(long timestamp) { internalProperties.put(LAST_ADMITTED_NOTIFICATION_TIMESTAMP_KEY, String.valueOf(timestamp)); return this; diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java deleted file mode 100644 index ac9d089f9..000000000 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableMetadataEntity.java +++ /dev/null @@ -1,75 +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.polaris.core.entity; - -import com.fasterxml.jackson.annotation.JsonIgnore; - -/** - * A {@link PolarisEntity} for storing table metadata. This can contain the raw content of the - * `metadata.json` or more granular information - */ -public class TableMetadataEntity extends PolarisEntity { - private static final String CONTENT_KEY = "content"; - private static final String METADATA_LOCATION_KEY = "metadata_location"; - - public TableMetadataEntity(PolarisBaseEntity sourceEntity) { - super(sourceEntity); - } - - public static TableMetadataEntity of(PolarisBaseEntity sourceEntity) { - if (sourceEntity != null) { - return new TableMetadataEntity(sourceEntity); - } - return null; - } - - @JsonIgnore - public String getContent() { - return getInternalPropertiesAsMap().get(CONTENT_KEY); - } - - @JsonIgnore - public String getMetadataLocation() { - return getInternalPropertiesAsMap().get(METADATA_LOCATION_KEY); - } - - public static class Builder - extends PolarisEntity.BaseBuilder { - public Builder() { - super(); - setType(PolarisEntityType.TABLE_METADATA); - } - - @Override - public TableMetadataEntity build() { - return new TableMetadataEntity(buildBase()); - } - - public TableMetadataEntity.Builder setContent(String content) { - this.internalProperties.put(CONTENT_KEY, content); - return this; - } - - public TableMetadataEntity.Builder setMetadataLocation(String metadataLocation) { - this.internalProperties.put(METADATA_LOCATION_KEY, metadataLocation); - this.setName(metadataLocation); - return this; - } - } -} diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 596838b42..acdb09bdc 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -823,18 +823,19 @@ public Map getCredentialConfig( } public TableMetadata loadTableMetadata(TableIdentifier identifier) { - boolean useMetadataCache = + long maxMetadataCacheBytes = callContext .getPolarisCallContext() .getConfigurationStore() .getConfiguration( - callContext.getPolarisCallContext(), PolarisConfiguration.METADATA_CACHE_ENABLED); - if (!useMetadataCache) { + callContext.getPolarisCallContext(), PolarisConfiguration.METADATA_CACHE_MAX_BYTES); + if (maxMetadataCacheBytes == PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING) { return loadTableMetadata(loadTable(identifier)); } else { Supplier fallback = () -> loadTableMetadata(loadTable(identifier)); return MetadataCacheManager.loadTableMetadata( identifier, + maxMetadataCacheBytes, callContext.getPolarisCallContext(), entityManager, resolvedEntityView, @@ -1890,10 +1891,6 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { } } - // Purge table metadata, if it exists: - MetadataCacheManager.dropTableMetadata( - identifier, callContext.getPolarisCallContext(), entityManager, resolvedEntityView); - // Drop the table: return getMetaStoreManager() .dropEntityIfExists( diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index c66ffac54..510fa64d4 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -18,7 +18,7 @@ */ package org.apache.polaris.service.persistence; -import java.util.Collection; +import java.nio.charset.StandardCharsets; import java.util.Optional; import java.util.function.Supplier; import org.apache.iceberg.Table; @@ -26,40 +26,51 @@ import org.apache.iceberg.TableMetadataParser; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.PolarisConfiguration; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; -import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.TableLikeEntity; -import org.apache.polaris.core.entity.TableMetadataEntity; import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; -import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class MetadataCacheManager { private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); - /** Load the cached {@link Table} or fall back to `fallback` if one doesn't exist */ + /** + * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist + * If the metadata is not currently cached, it may be added to the cache. + */ public static TableMetadata loadTableMetadata( TableIdentifier tableIdentifier, + long maxBytesToCache, PolarisCallContext callContext, PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView, Supplier fallback) { LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); - Optional cachedMetadata = - loadCachedTableMetadata(tableIdentifier, callContext, entityManager, resolvedEntityView); - if (cachedMetadata.isPresent()) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); + TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); + Optional cachedMetadata = Optional + .ofNullable(tableLikeEntity) + .map(TableLikeEntity::getMetadataContent) + .map(TableMetadataParser::fromJson); + boolean isCacheValid = cachedMetadata + .map(TableMetadata::metadataFileLocation) + .map(s -> s.equals(tableLikeEntity.getMetadataLocation())) + .orElse(false); + if (isCacheValid) { LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); return cachedMetadata.get(); } else { TableMetadata metadata = fallback.get(); PolarisMetaStoreManager.EntityResult cacheResult = cacheTableMetadata( - tableIdentifier, metadata, callContext, entityManager, resolvedEntityView); + tableLikeEntity, metadata, maxBytesToCache, callContext, entityManager, resolvedEntityView); if (!cacheResult.isSuccess()) { LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); } @@ -73,120 +84,55 @@ public static TableMetadata loadTableMetadata( * @return The result of trying to cache the metadata */ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( - TableIdentifier tableIdentifier, + TableLikeEntity tableLikeEntity, TableMetadata metadata, + long maxBytesToCache, PolarisCallContext callContext, PolarisEntityManager entityManager, PolarisResolutionManifestCatalogView resolvedEntityView) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); - if (resolvedEntities == null) { - return new PolarisMetaStoreManager.EntityResult( - PolarisMetaStoreManager.ReturnStatus.ENTITY_NOT_FOUND, null); - } else { - TableLikeEntity tableEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - TableMetadataEntity tableMetadataEntity = - new TableMetadataEntity.Builder() - .setCatalogId(tableEntity.getCatalogId()) - .setParentId(tableEntity.getId()) - .setId(entityManager.getMetaStoreManager().generateNewEntityId(callContext).getId()) - .setCreateTimestamp(System.currentTimeMillis()) - .setMetadataLocation(metadata.metadataFileLocation()) - .setContent(TableMetadataParser.toJson(metadata)) - .build(); - try { - return entityManager - .getMetaStoreManager() - .createEntityIfNotExists( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - tableMetadataEntity); - } catch (RuntimeException e) { - // PersistenceException (& other extension-specific exceptions) may not be in scope, - // but we can make a best-effort attempt to swallow it and just forego caching - if (e.toString().contains("PersistenceException")) { - return new PolarisMetaStoreManager.EntityResult( - PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, e.getMessage()); - } else { - throw e; + String json = TableMetadataParser.toJson(metadata); + // We should not reach this method in this case, but check just in case... + if (maxBytesToCache != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING) { + long sizeInBytes = json.getBytes(StandardCharsets.UTF_8).length; + if (sizeInBytes > maxBytesToCache) { + LOGGER.debug(String.format( + "Will not cache metadata for %s; metadata is %d bytes and the limit is %d", + tableLikeEntity.getTableIdentifier(), + sizeInBytes, + maxBytesToCache + )); + return new PolarisMetaStoreManager.EntityResult(PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); + } else { + LOGGER.debug(String.format("Caching metadata for %s", tableLikeEntity.getTableIdentifier())); + TableLikeEntity newTableLikeEntity = new TableLikeEntity.Builder(tableLikeEntity) + .setMetadataContent(json) + .build(); + PolarisResolvedPathWrapper resolvedPath = + resolvedEntityView.getResolvedPath(tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); + try { + return entityManager.getMetaStoreManager().updateEntityPropertiesIfNotChanged( + callContext, + PolarisEntity.toCoreList(resolvedPath.getRawFullPath()), + newTableLikeEntity); + } catch (RuntimeException e) { + // PersistenceException (& other extension-specific exceptions) may not be in scope, + // but we can make a best-effort attempt to swallow it and just forego caching + if (e.toString().contains("PersistenceException")) { + LOGGER.debug(String.format( + "Encountered an error while caching %s: %s", tableLikeEntity.getTableIdentifier(), e)); + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, e.getMessage()); + } else { + throw e; + } } } - } - } - - /** Return the cached {@link Table} entity, if one exists */ - private static @NotNull Optional loadCachedTableMetadata( - TableIdentifier tableIdentifier, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView) { - return loadCachedTableMetadataImpl( - tableIdentifier, callContext, entityManager, resolvedEntityView, false); - } - - public static void dropTableMetadata( - TableIdentifier tableIdentifier, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView) { - loadCachedTableMetadataImpl( - tableIdentifier, callContext, entityManager, resolvedEntityView, true); - } - - /** Return the cached {@link Table} entity, if one exists */ - private static @NotNull Optional loadCachedTableMetadataImpl( - TableIdentifier tableIdentifier, - PolarisCallContext callContext, - PolarisEntityManager entityManager, - PolarisResolutionManifestCatalogView resolvedEntityView, - boolean dropEverything) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); - if (resolvedEntities == null) { - return Optional.empty(); } else { - TableLikeEntity entity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - String metadataLocation = entity.getMetadataLocation(); - PolarisMetaStoreManager.ListEntitiesResult metadataResult = - entityManager - .getMetaStoreManager() - .listEntities( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - PolarisEntityType.TABLE_METADATA, - PolarisEntitySubType.ANY_SUBTYPE); - return Optional.ofNullable(metadataResult.getEntities()).stream() - .flatMap(Collection::stream) - .flatMap( - result -> { - PolarisMetaStoreManager.EntityResult metadataEntityResult = - entityManager - .getMetaStoreManager() - .loadEntity(callContext, result.getCatalogId(), result.getId()); - return Optional.ofNullable(metadataEntityResult.getEntity()) - .map(TableMetadataEntity::of) - .stream(); - }) - .filter( - metadata -> { - if (metadata.getMetadataLocation().equals(metadataLocation) && !dropEverything) { - return true; - } else { - LOGGER.debug( - String.format("Deleting old entry for %s", metadata.getMetadataLocation())); - entityManager - .getMetaStoreManager() - .dropEntityIfExists( - callContext, - PolarisEntity.toCoreList(resolvedEntities.getRawFullPath()), - metadata, - null, - /* purge= */ false); - return false; - } - }) - .findFirst() - .map(metadataEntity -> TableMetadataParser.fromJson(metadataEntity.getContent())); + LOGGER.debug(String.format( + "Will not cache metadata for %s; metadata caching is disabled", + tableLikeEntity.getTableIdentifier() + )); + return new PolarisMetaStoreManager.EntityResult(PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); } } } diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index e9daa99eb..0502eea28 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -35,6 +35,7 @@ import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import org.apache.commons.lang3.NotImplementedException; import org.apache.iceberg.BaseTable; @@ -1555,6 +1556,7 @@ public void testMetadataCachingWithManualFallback() { TableMetadata loadedMetadata = MetadataCacheManager.loadTableMetadata( tableIdentifier, + Long.MAX_VALUE, polarisContext, entityManager, passthroughView, @@ -1566,6 +1568,7 @@ public void testMetadataCachingWithManualFallback() { TableMetadata cachedMetadata = MetadataCacheManager.loadTableMetadata( tableIdentifier, + Long.MAX_VALUE, polarisContext, entityManager, passthroughView, @@ -1585,18 +1588,22 @@ public void testMetadataCachingWithManualFallback() { TableMetadata updatedMetadata = tableOps.current().updateLocation(originalMetadata.location() + "-updated"); tableOps.commit(tableOps.current(), updatedMetadata); - TableMetadata spyUpdatedMetadata = Mockito.spy(updatedMetadata); - Mockito.doReturn("spy-location").when(spyUpdatedMetadata).metadataFileLocation(); + AtomicBoolean wasFallbackCalledAgain = new AtomicBoolean(false); // Read from the cache; it should detect a chance due to the update and load the new fallback TableMetadata reloadedMetadata = MetadataCacheManager.loadTableMetadata( tableIdentifier, + Long.MAX_VALUE, polarisContext, entityManager, passthroughView, - () -> spyUpdatedMetadata); + () -> { + wasFallbackCalledAgain.set(true); + return updatedMetadata; + }); - Assertions.assertThat(reloadedMetadata).isSameAs(spyUpdatedMetadata); + Assertions.assertThat(reloadedMetadata).isNotSameAs(cachedMetadata); + Assertions.assertThat(wasFallbackCalledAgain.get()).isTrue(); } } From 0b3df37c122bf6d9c2d76251d62e435d952d8058 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 17:30:21 -0800 Subject: [PATCH 21/40] autolint --- .../polaris/core/PolarisConfiguration.java | 18 ++--- .../persistence/MetadataCacheManager.java | 79 +++++++++++-------- 2 files changed, 54 insertions(+), 43 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java index 86985f44e..8a6713221 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.Optional; import java.util.function.Function; - import org.apache.polaris.core.admin.model.StorageConfigInfo; public class PolarisConfiguration { @@ -68,12 +67,13 @@ T cast(Object value) { } private void validate(T value) { - this.validation.ifPresent(v -> { - if (!v.apply(value)) { - throw new IllegalArgumentException( - String.format("Configuration %s has invalid value %s", key, defaultValue)); - } - }); + this.validation.ifPresent( + v -> { + if (!v.apply(value)) { + throw new IllegalArgumentException( + String.format("Configuration %s has invalid value %s", key, defaultValue)); + } + }); } public static class Builder { @@ -232,8 +232,8 @@ public static Builder builder() { PolarisConfiguration.builder() .key("METADATA_CACHE_MAX_BYTES") .description( - "If nonzero, the max size a table's metadata can be in order to be cached in the persistence layer." + - " If zero, no metadata will be cached or served from the cache. If -1, all metadata will be cached.") + "If nonzero, the max size a table's metadata can be in order to be cached in the persistence layer." + + " If zero, no metadata will be cached or served from the cache. If -1, all metadata will be cached.") .defaultValue(METADATA_CACHE_MAX_BYTES_NO_CACHING) .validation(value -> value >= -1) .build(); diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 510fa64d4..d7fbb1936 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -41,8 +41,8 @@ public class MetadataCacheManager { private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); /** - * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist - * If the metadata is not currently cached, it may be added to the cache. + * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist If the metadata + * is not currently cached, it may be added to the cache. */ public static TableMetadata loadTableMetadata( TableIdentifier tableIdentifier, @@ -55,14 +55,15 @@ public static TableMetadata loadTableMetadata( PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - Optional cachedMetadata = Optional - .ofNullable(tableLikeEntity) - .map(TableLikeEntity::getMetadataContent) - .map(TableMetadataParser::fromJson); - boolean isCacheValid = cachedMetadata - .map(TableMetadata::metadataFileLocation) - .map(s -> s.equals(tableLikeEntity.getMetadataLocation())) - .orElse(false); + Optional cachedMetadata = + Optional.ofNullable(tableLikeEntity) + .map(TableLikeEntity::getMetadataContent) + .map(TableMetadataParser::fromJson); + boolean isCacheValid = + cachedMetadata + .map(TableMetadata::metadataFileLocation) + .map(s -> s.equals(tableLikeEntity.getMetadataLocation())) + .orElse(false); if (isCacheValid) { LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); return cachedMetadata.get(); @@ -70,7 +71,12 @@ public static TableMetadata loadTableMetadata( TableMetadata metadata = fallback.get(); PolarisMetaStoreManager.EntityResult cacheResult = cacheTableMetadata( - tableLikeEntity, metadata, maxBytesToCache, callContext, entityManager, resolvedEntityView); + tableLikeEntity, + metadata, + maxBytesToCache, + callContext, + entityManager, + resolvedEntityView); if (!cacheResult.isSuccess()) { LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); } @@ -95,31 +101,35 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( if (maxBytesToCache != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING) { long sizeInBytes = json.getBytes(StandardCharsets.UTF_8).length; if (sizeInBytes > maxBytesToCache) { - LOGGER.debug(String.format( - "Will not cache metadata for %s; metadata is %d bytes and the limit is %d", - tableLikeEntity.getTableIdentifier(), - sizeInBytes, - maxBytesToCache - )); - return new PolarisMetaStoreManager.EntityResult(PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); + LOGGER.debug( + String.format( + "Will not cache metadata for %s; metadata is %d bytes and the limit is %d", + tableLikeEntity.getTableIdentifier(), sizeInBytes, maxBytesToCache)); + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); } else { - LOGGER.debug(String.format("Caching metadata for %s", tableLikeEntity.getTableIdentifier())); - TableLikeEntity newTableLikeEntity = new TableLikeEntity.Builder(tableLikeEntity) - .setMetadataContent(json) - .build(); + LOGGER.debug( + String.format("Caching metadata for %s", tableLikeEntity.getTableIdentifier())); + TableLikeEntity newTableLikeEntity = + new TableLikeEntity.Builder(tableLikeEntity).setMetadataContent(json).build(); PolarisResolvedPathWrapper resolvedPath = - resolvedEntityView.getResolvedPath(tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); + resolvedEntityView.getResolvedPath( + tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); try { - return entityManager.getMetaStoreManager().updateEntityPropertiesIfNotChanged( - callContext, - PolarisEntity.toCoreList(resolvedPath.getRawFullPath()), - newTableLikeEntity); + return entityManager + .getMetaStoreManager() + .updateEntityPropertiesIfNotChanged( + callContext, + PolarisEntity.toCoreList(resolvedPath.getRawFullPath()), + newTableLikeEntity); } catch (RuntimeException e) { // PersistenceException (& other extension-specific exceptions) may not be in scope, // but we can make a best-effort attempt to swallow it and just forego caching if (e.toString().contains("PersistenceException")) { - LOGGER.debug(String.format( - "Encountered an error while caching %s: %s", tableLikeEntity.getTableIdentifier(), e)); + LOGGER.debug( + String.format( + "Encountered an error while caching %s: %s", + tableLikeEntity.getTableIdentifier(), e)); return new PolarisMetaStoreManager.EntityResult( PolarisMetaStoreManager.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, e.getMessage()); } else { @@ -128,11 +138,12 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } } } else { - LOGGER.debug(String.format( - "Will not cache metadata for %s; metadata caching is disabled", - tableLikeEntity.getTableIdentifier() - )); - return new PolarisMetaStoreManager.EntityResult(PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); + LOGGER.debug( + String.format( + "Will not cache metadata for %s; metadata caching is disabled", + tableLikeEntity.getTableIdentifier())); + return new PolarisMetaStoreManager.EntityResult( + PolarisMetaStoreManager.ReturnStatus.SUCCESS, null); } } } From 5f7c74f3081a226a1eff7518260e64f7822b21ac Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 19:04:34 -0800 Subject: [PATCH 22/40] fixes --- .../service/persistence/MetadataCacheManager.java | 11 +++-------- .../service/catalog/BasePolarisCatalogTest.java | 9 +++------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index d7fbb1936..39f96a369 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -53,18 +53,13 @@ public static TableMetadata loadTableMetadata( Supplier fallback) { LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); + resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); Optional cachedMetadata = Optional.ofNullable(tableLikeEntity) .map(TableLikeEntity::getMetadataContent) .map(TableMetadataParser::fromJson); - boolean isCacheValid = - cachedMetadata - .map(TableMetadata::metadataFileLocation) - .map(s -> s.equals(tableLikeEntity.getMetadataLocation())) - .orElse(false); - if (isCacheValid) { + if (cachedMetadata.isPresent()) { LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); return cachedMetadata.get(); } else { @@ -120,7 +115,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( .getMetaStoreManager() .updateEntityPropertiesIfNotChanged( callContext, - PolarisEntity.toCoreList(resolvedPath.getRawFullPath()), + PolarisEntity.toCoreList(resolvedPath.getRawParentPath()), newTableLikeEntity); } catch (RuntimeException e) { // PersistenceException (& other extension-specific exceptions) may not be in scope, diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index 0502eea28..9419f07ea 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -100,6 +100,7 @@ import org.apache.polaris.service.types.NotificationRequest; import org.apache.polaris.service.types.NotificationType; import org.apache.polaris.service.types.TableUpdateNotification; +import org.apache.spark.Partition; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.Assertions; import org.jetbrains.annotations.Nullable; @@ -1537,11 +1538,6 @@ private Schema buildSchema(int fields) { return new Schema(fieldsArray); } - private TableMetadata createTableMetadata(String location, Schema schema) { - return TableMetadata.newTableMetadata( - schema, PartitionSpec.unpartitioned(), location, Collections.emptyMap()); - } - @Test public void testMetadataCachingWithManualFallback() { Namespace namespace = Namespace.of("manual-namespace"); @@ -1586,7 +1582,7 @@ public void testMetadataCachingWithManualFallback() { // Update the table TableOperations tableOps = catalog.newTableOps(tableIdentifier); TableMetadata updatedMetadata = - tableOps.current().updateLocation(originalMetadata.location() + "-updated"); + tableOps.current().updateSchema(buildSchema(100), 100); tableOps.commit(tableOps.current(), updatedMetadata); AtomicBoolean wasFallbackCalledAgain = new AtomicBoolean(false); @@ -1604,6 +1600,7 @@ public void testMetadataCachingWithManualFallback() { }); Assertions.assertThat(reloadedMetadata).isNotSameAs(cachedMetadata); + Assertions.assertThat(reloadedMetadata.schema().columns().size()).isEqualTo(100); Assertions.assertThat(wasFallbackCalledAgain.get()).isTrue(); } } From d99a7965e07a7ea298a05ca928efa18239695e2a Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 19:04:39 -0800 Subject: [PATCH 23/40] autolint --- .../polaris/service/catalog/BasePolarisCatalogTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index 9419f07ea..af038758c 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -28,7 +28,6 @@ import java.io.IOException; import java.time.Clock; import java.util.Arrays; -import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.List; @@ -100,7 +99,6 @@ import org.apache.polaris.service.types.NotificationRequest; import org.apache.polaris.service.types.NotificationType; import org.apache.polaris.service.types.TableUpdateNotification; -import org.apache.spark.Partition; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.Assertions; import org.jetbrains.annotations.Nullable; @@ -1581,8 +1579,7 @@ public void testMetadataCachingWithManualFallback() { // Update the table TableOperations tableOps = catalog.newTableOps(tableIdentifier); - TableMetadata updatedMetadata = - tableOps.current().updateSchema(buildSchema(100), 100); + TableMetadata updatedMetadata = tableOps.current().updateSchema(buildSchema(100), 100); tableOps.commit(tableOps.current(), updatedMetadata); AtomicBoolean wasFallbackCalledAgain = new AtomicBoolean(false); From f1023eca8e384aeca4e4d10d24bdd68b9ddd112f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 19:10:20 -0800 Subject: [PATCH 24/40] stable --- .../polaris/core/entity/TableLikeEntity.java | 22 ++++++++++++++----- .../persistence/MetadataCacheManager.java | 16 +++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java index fb026c24c..1d6b67dc1 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java @@ -30,8 +30,12 @@ public class TableLikeEntity extends PolarisEntity { public static final String METADATA_LOCATION_KEY = "metadata-location"; // For applicable types, this key on the "internalProperties" map will return the content of the - // metadata.json file located at `METADATA_LOCATION_KEY` - private static final String METADATA_CONTENT_KEY = "metadata-content"; + // metadata.json file located at `METADATA_CACHE_LOCATION_KEY` + private static final String METADATA_CACHE_CONTENT_KEY = "metadata-cache-content"; + + // For applicable types, this key on the "internalProperties" map will return the location of the + // `metadata.json` that is cached in METADATA_CACHE_CONTENT_KEY + private static final String METADATA_CACHE_LOCATION_KEY = "metadata-cache-location"; public static final String USER_SPECIFIED_WRITE_DATA_LOCATION_KEY = "write.data.path"; public static final String USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY = "write.metadata.path"; @@ -72,8 +76,13 @@ public String getMetadataLocation() { } @JsonIgnore - public String getMetadataContent() { - return getInternalPropertiesAsMap().get(METADATA_CONTENT_KEY); + public String getMetadataCacheContent() { + return getInternalPropertiesAsMap().get(METADATA_CACHE_CONTENT_KEY); + } + + @JsonIgnore + public String getMetadataCacheLocationKey() { + return getInternalPropertiesAsMap().get(METADATA_CACHE_LOCATION_KEY); } @JsonIgnore @@ -130,8 +139,9 @@ public Builder setMetadataLocation(String location) { return this; } - public Builder setMetadataContent(String content) { - internalProperties.put(METADATA_CONTENT_KEY, content); + public Builder setMetadataContent(String location, String content) { + internalProperties.put(METADATA_CACHE_LOCATION_KEY, location); + internalProperties.put(METADATA_CACHE_CONTENT_KEY, content); return this; } diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 39f96a369..9171accf6 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -55,13 +55,12 @@ public static TableMetadata loadTableMetadata( PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - Optional cachedMetadata = - Optional.ofNullable(tableLikeEntity) - .map(TableLikeEntity::getMetadataContent) - .map(TableMetadataParser::fromJson); - if (cachedMetadata.isPresent()) { + boolean isCacheValid = tableLikeEntity + .getMetadataLocation() + .equals(tableLikeEntity.getMetadataCacheLocationKey()); + if (isCacheValid) { LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); - return cachedMetadata.get(); + return TableMetadataParser.fromJson(tableLikeEntity.getMetadataCacheContent()); } else { TableMetadata metadata = fallback.get(); PolarisMetaStoreManager.EntityResult cacheResult = @@ -105,8 +104,9 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } else { LOGGER.debug( String.format("Caching metadata for %s", tableLikeEntity.getTableIdentifier())); - TableLikeEntity newTableLikeEntity = - new TableLikeEntity.Builder(tableLikeEntity).setMetadataContent(json).build(); + TableLikeEntity newTableLikeEntity = new TableLikeEntity.Builder(tableLikeEntity) + .setMetadataContent(tableLikeEntity.getMetadataLocation(), json) + .build(); PolarisResolvedPathWrapper resolvedPath = resolvedEntityView.getResolvedPath( tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); From 045d93db9ff345f445537537aeac2059151cec83 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 19:10:24 -0800 Subject: [PATCH 25/40] autolint --- .../service/persistence/MetadataCacheManager.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 9171accf6..7a22400e4 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -19,7 +19,6 @@ package org.apache.polaris.service.persistence; import java.nio.charset.StandardCharsets; -import java.util.Optional; import java.util.function.Supplier; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; @@ -55,9 +54,8 @@ public static TableMetadata loadTableMetadata( PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); - boolean isCacheValid = tableLikeEntity - .getMetadataLocation() - .equals(tableLikeEntity.getMetadataCacheLocationKey()); + boolean isCacheValid = + tableLikeEntity.getMetadataLocation().equals(tableLikeEntity.getMetadataCacheLocationKey()); if (isCacheValid) { LOGGER.debug(String.format("Using cached metadata for %s", tableIdentifier)); return TableMetadataParser.fromJson(tableLikeEntity.getMetadataCacheContent()); @@ -104,9 +102,10 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( } else { LOGGER.debug( String.format("Caching metadata for %s", tableLikeEntity.getTableIdentifier())); - TableLikeEntity newTableLikeEntity = new TableLikeEntity.Builder(tableLikeEntity) - .setMetadataContent(tableLikeEntity.getMetadataLocation(), json) - .build(); + TableLikeEntity newTableLikeEntity = + new TableLikeEntity.Builder(tableLikeEntity) + .setMetadataContent(tableLikeEntity.getMetadataLocation(), json) + .build(); PolarisResolvedPathWrapper resolvedPath = resolvedEntityView.getResolvedPath( tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); From 20edca50dc6898bb130faac45b7ce1bcf17fdb52 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 19:14:34 -0800 Subject: [PATCH 26/40] polish --- .../polaris/service/persistence/MetadataCacheManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 7a22400e4..87ad91436 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -40,7 +40,7 @@ public class MetadataCacheManager { private static final Logger LOGGER = LoggerFactory.getLogger(MetadataCacheManager.class); /** - * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist If the metadata + * Load the cached {@link Table} or fall back to `fallback` if one doesn't exist. If the metadata * is not currently cached, it may be added to the cache. */ public static TableMetadata loadTableMetadata( @@ -52,7 +52,7 @@ public static TableMetadata loadTableMetadata( Supplier fallback) { LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); + resolvedEntityView.getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE); TableLikeEntity tableLikeEntity = TableLikeEntity.of(resolvedEntities.getRawLeafEntity()); boolean isCacheValid = tableLikeEntity.getMetadataLocation().equals(tableLikeEntity.getMetadataCacheLocationKey()); From f189e986c0fe8765b1b5c176e97deffd6cd37b0e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 22:08:05 -0800 Subject: [PATCH 27/40] one fix --- .../polaris/service/catalog/BasePolarisCatalog.java | 2 +- .../service/persistence/MetadataCacheManager.java | 9 ++++----- .../polaris/service/catalog/BasePolarisCatalogTest.java | 6 +++--- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index acdb09bdc..2fc684304 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -837,7 +837,7 @@ public TableMetadata loadTableMetadata(TableIdentifier identifier) { identifier, maxMetadataCacheBytes, callContext.getPolarisCallContext(), - entityManager, + entityManager.getMetaStoreManager(), resolvedEntityView, fallback); } diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index 87ad91436..b4af2a867 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -47,7 +47,7 @@ public static TableMetadata loadTableMetadata( TableIdentifier tableIdentifier, long maxBytesToCache, PolarisCallContext callContext, - PolarisEntityManager entityManager, + PolarisMetaStoreManager metastoreManager, PolarisResolutionManifestCatalogView resolvedEntityView, Supplier fallback) { LOGGER.debug(String.format("Loading cached metadata for %s", tableIdentifier)); @@ -67,7 +67,7 @@ public static TableMetadata loadTableMetadata( metadata, maxBytesToCache, callContext, - entityManager, + metastoreManager, resolvedEntityView); if (!cacheResult.isSuccess()) { LOGGER.debug(String.format("Failed to cache metadata for %s", tableIdentifier)); @@ -86,7 +86,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( TableMetadata metadata, long maxBytesToCache, PolarisCallContext callContext, - PolarisEntityManager entityManager, + PolarisMetaStoreManager metaStoreManager, PolarisResolutionManifestCatalogView resolvedEntityView) { String json = TableMetadataParser.toJson(metadata); // We should not reach this method in this case, but check just in case... @@ -110,8 +110,7 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( resolvedEntityView.getResolvedPath( tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); try { - return entityManager - .getMetaStoreManager() + return metaStoreManager .updateEntityPropertiesIfNotChanged( callContext, PolarisEntity.toCoreList(resolvedPath.getRawParentPath()), diff --git a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java index af038758c..5f9b27219 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java @@ -1552,7 +1552,7 @@ public void testMetadataCachingWithManualFallback() { tableIdentifier, Long.MAX_VALUE, polarisContext, - entityManager, + metaStoreManager, passthroughView, () -> originalMetadata); @@ -1564,7 +1564,7 @@ public void testMetadataCachingWithManualFallback() { tableIdentifier, Long.MAX_VALUE, polarisContext, - entityManager, + metaStoreManager, passthroughView, () -> { throw new IllegalStateException("Fell back even though a cache entry should exist!"); @@ -1589,7 +1589,7 @@ public void testMetadataCachingWithManualFallback() { tableIdentifier, Long.MAX_VALUE, polarisContext, - entityManager, + metaStoreManager, passthroughView, () -> { wasFallbackCalledAgain.set(true); From 5e90ba229112ae17ee66e6efaf4eebbf0cff74ef Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 22:08:09 -0800 Subject: [PATCH 28/40] autolint --- .../service/persistence/MetadataCacheManager.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java index b4af2a867..f9712a70b 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/persistence/MetadataCacheManager.java @@ -29,7 +29,6 @@ import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.TableLikeEntity; -import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; @@ -110,11 +109,10 @@ private static PolarisMetaStoreManager.EntityResult cacheTableMetadata( resolvedEntityView.getResolvedPath( tableLikeEntity.getTableIdentifier(), PolarisEntitySubType.TABLE); try { - return metaStoreManager - .updateEntityPropertiesIfNotChanged( - callContext, - PolarisEntity.toCoreList(resolvedPath.getRawParentPath()), - newTableLikeEntity); + return metaStoreManager.updateEntityPropertiesIfNotChanged( + callContext, + PolarisEntity.toCoreList(resolvedPath.getRawParentPath()), + newTableLikeEntity); } catch (RuntimeException e) { // PersistenceException (& other extension-specific exceptions) may not be in scope, // but we can make a best-effort attempt to swallow it and just forego caching From fbdce6ae86f2fd65d9d5f98921f39e2acca99b0b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 11 Nov 2024 22:12:59 -0800 Subject: [PATCH 29/40] ? --- .../org/apache/polaris/service/catalog/BasePolarisCatalog.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 2fc684304..8fb2e35f8 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -837,7 +837,7 @@ public TableMetadata loadTableMetadata(TableIdentifier identifier) { identifier, maxMetadataCacheBytes, callContext.getPolarisCallContext(), - entityManager.getMetaStoreManager(), + metaStoreManager, resolvedEntityView, fallback); } From f1461e546713d8edb81bd952846453f4de7ed097 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 13 Nov 2024 22:24:17 -0800 Subject: [PATCH 30/40] more widespread use of loadTableMetadata --- .../catalog/PolarisCatalogHandlerWrapper.java | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index ef2dff86f..443c894f9 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -612,7 +612,14 @@ public LoadTableResponse createTableDirectWithWriteDelegation( .create(); if (table instanceof BaseTable baseTable) { - TableMetadata tableMetadata = baseTable.operations().current(); + + final TableMetadata tableMetadata; + if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { + tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); + } else { + tableMetadata = baseTable.operations().current(); + } + LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(tableMetadata); if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { @@ -1029,20 +1036,28 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest) commitTransactionRequest.tableChanges().stream() .forEach( change -> { - Table table = baseCatalog.loadTable(change.identifier()); + + final Table table = baseCatalog.loadTable(change.identifier()); + if (!(table instanceof BaseTable)) { throw new IllegalStateException( "Cannot wrap catalog that does not produce BaseTable"); } + + TableOperations tableOps = ((BaseTable) table).operations(); + final TableMetadata currentMetadata; + if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { + currentMetadata = basePolarisCatalog.loadTableMetadata(change.identifier()); + } else { + currentMetadata = tableOps.current(); + } + if (isCreate(change)) { throw new BadRequestException( "Unsupported operation: commitTranaction with updateForStagedCreate: %s", change); } - TableOperations tableOps = ((BaseTable) table).operations(); - TableMetadata currentMetadata = tableOps.current(); - // Validate requirements; any CommitFailedExceptions will fail the overall request change.requirements().forEach(requirement -> requirement.validate(currentMetadata)); From c5081d5746568ed2da64323eda9a8bdaead220ea Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 13 Nov 2024 22:24:29 -0800 Subject: [PATCH 31/40] autolint --- .../polaris/service/catalog/PolarisCatalogHandlerWrapper.java | 1 - 1 file changed, 1 deletion(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 443c894f9..25623c20c 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -1036,7 +1036,6 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest) commitTransactionRequest.tableChanges().stream() .forEach( change -> { - final Table table = baseCatalog.loadTable(change.identifier()); if (!(table instanceof BaseTable)) { From 452c5a151d7794044893273021bcbe2cdc975237 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 13 Nov 2024 22:59:20 -0800 Subject: [PATCH 32/40] persist on write --- .../service/catalog/BasePolarisCatalog.java | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 8fb2e35f8..78c2cae7b 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableMap; import java.io.Closeable; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -1372,23 +1373,37 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { TableLikeEntity entity = TableLikeEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); String existingLocation; + long maxMetadataCacheBytes = + callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), PolarisConfiguration.METADATA_CACHE_MAX_BYTES); + String metadataJson = TableMetadataParser.toJson(metadata); + boolean shouldPersistMetadata = + maxMetadataCacheBytes != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING && + metadataJson.getBytes(StandardCharsets.UTF_8).length <= maxMetadataCacheBytes; if (null == entity) { existingLocation = null; - entity = - new TableLikeEntity.Builder(tableIdentifier, newLocation) - .setCatalogId(getCatalogId()) - .setSubType(PolarisEntitySubType.TABLE) - .setBaseLocation(metadata.location()) - .setId( - getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) - .build(); + var builder = new TableLikeEntity.Builder(tableIdentifier, newLocation) + .setCatalogId(getCatalogId()) + .setSubType(PolarisEntitySubType.TABLE) + .setBaseLocation(metadata.location()) + .setId( + getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()); + if (shouldPersistMetadata) { + builder.setMetadataContent(newLocation, metadataJson); + } + entity = builder.build(); } else { existingLocation = entity.getMetadataLocation(); - entity = - new TableLikeEntity.Builder(entity) + var builder = new TableLikeEntity.Builder(entity) .setBaseLocation(metadata.location()) - .setMetadataLocation(newLocation) - .build(); + .setMetadataLocation(newLocation); + if (shouldPersistMetadata) { + builder.setMetadataContent(newLocation, metadataJson); + } + entity = builder.build(); } if (!Objects.equal(existingLocation, oldLocation)) { if (null == base) { From b85ad92d3526bb167633dc02b11bac8fac842890 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 13 Nov 2024 22:59:24 -0800 Subject: [PATCH 33/40] autolint --- .../service/catalog/BasePolarisCatalog.java | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 78c2cae7b..e51019139 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1378,26 +1378,29 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { .getPolarisCallContext() .getConfigurationStore() .getConfiguration( - callContext.getPolarisCallContext(), PolarisConfiguration.METADATA_CACHE_MAX_BYTES); + callContext.getPolarisCallContext(), + PolarisConfiguration.METADATA_CACHE_MAX_BYTES); String metadataJson = TableMetadataParser.toJson(metadata); boolean shouldPersistMetadata = - maxMetadataCacheBytes != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING && - metadataJson.getBytes(StandardCharsets.UTF_8).length <= maxMetadataCacheBytes; + maxMetadataCacheBytes != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING + && metadataJson.getBytes(StandardCharsets.UTF_8).length <= maxMetadataCacheBytes; if (null == entity) { existingLocation = null; - var builder = new TableLikeEntity.Builder(tableIdentifier, newLocation) - .setCatalogId(getCatalogId()) - .setSubType(PolarisEntitySubType.TABLE) - .setBaseLocation(metadata.location()) - .setId( - getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()); + var builder = + new TableLikeEntity.Builder(tableIdentifier, newLocation) + .setCatalogId(getCatalogId()) + .setSubType(PolarisEntitySubType.TABLE) + .setBaseLocation(metadata.location()) + .setId( + getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()); if (shouldPersistMetadata) { builder.setMetadataContent(newLocation, metadataJson); } entity = builder.build(); } else { existingLocation = entity.getMetadataLocation(); - var builder = new TableLikeEntity.Builder(entity) + var builder = + new TableLikeEntity.Builder(entity) .setBaseLocation(metadata.location()) .setMetadataLocation(newLocation); if (shouldPersistMetadata) { From fde3910e86c58e05436cb4e4e69c9e0734df6528 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 12:53:47 -0800 Subject: [PATCH 34/40] debugging --- .../service/catalog/BasePolarisCatalog.java | 17 +++++++---------- .../catalog/PolarisCatalogHandlerWrapper.java | 4 +++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index e51019139..b2258e9c8 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1384,30 +1384,27 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { boolean shouldPersistMetadata = maxMetadataCacheBytes != PolarisConfiguration.METADATA_CACHE_MAX_BYTES_NO_CACHING && metadataJson.getBytes(StandardCharsets.UTF_8).length <= maxMetadataCacheBytes; + final TableLikeEntity.Builder builder; if (null == entity) { existingLocation = null; - var builder = + builder = new TableLikeEntity.Builder(tableIdentifier, newLocation) .setCatalogId(getCatalogId()) .setSubType(PolarisEntitySubType.TABLE) .setBaseLocation(metadata.location()) .setId( getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()); - if (shouldPersistMetadata) { - builder.setMetadataContent(newLocation, metadataJson); - } - entity = builder.build(); } else { existingLocation = entity.getMetadataLocation(); - var builder = + builder = new TableLikeEntity.Builder(entity) .setBaseLocation(metadata.location()) .setMetadataLocation(newLocation); - if (shouldPersistMetadata) { - builder.setMetadataContent(newLocation, metadataJson); - } - entity = builder.build(); } + if (shouldPersistMetadata) { + builder.setMetadataContent(newLocation, metadataJson); + } + entity = builder.build(); if (!Objects.equal(existingLocation, oldLocation)) { if (null == base) { throw new AlreadyExistsException("Table already exists: %s", tableName()); diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 25623c20c..e829e5cdd 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -615,7 +615,9 @@ public LoadTableResponse createTableDirectWithWriteDelegation( final TableMetadata tableMetadata; if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { - tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); + // tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); + // TODO debug failures for diffAgainstSingleTable + tableMetadata = baseTable.operations().current(); } else { tableMetadata = baseTable.operations().current(); } From 34b9b0fe245ef3b6548acf3b29816852ae8dde03 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 13:11:49 -0800 Subject: [PATCH 35/40] fix failing test, add TODO --- .../service/catalog/PolarisCatalogHandlerWrapper.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index e829e5cdd..0bf2b60cc 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -615,9 +615,7 @@ public LoadTableResponse createTableDirectWithWriteDelegation( final TableMetadata tableMetadata; if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { - // tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); - // TODO debug failures for diffAgainstSingleTable - tableMetadata = baseTable.operations().current(); + tableMetadata = basePolarisCatalog.loadTableMetadata(tableIdentifier); } else { tableMetadata = baseTable.operations().current(); } @@ -1048,7 +1046,9 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest) TableOperations tableOps = ((BaseTable) table).operations(); final TableMetadata currentMetadata; if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { - currentMetadata = basePolarisCatalog.loadTableMetadata(change.identifier()); + // TODO use cached metadata once the check in BaseMetastoreTableOperations is fixed + // currentMetadata = basePolarisCatalog.loadTableMetadata(change.identifier()); + currentMetadata = tableOps.current(); } else { currentMetadata = tableOps.current(); } From 6bc07c8c0f3b63af0f90291a638e732c52542aab Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 13:19:58 -0800 Subject: [PATCH 36/40] debugging --- .../service/catalog/BasePolarisCatalog.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index b2258e9c8..eaccc1d31 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1426,6 +1426,35 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { } } + @Override + public void commit(TableMetadata base, TableMetadata metadata) { + // if the metadata is already out of date, reject it + if (base.metadataFileLocation() != current().metadataFileLocation()) { + if (base != null) { + throw new CommitFailedException("Cannot commit: stale table metadata"); + } else { + // when current is non-null, the table exists. but when base is null, the commit is trying + // to create the table + throw new AlreadyExistsException("Table already exists: %s", tableName()); + } + } + // if the metadata is not changed, return early + if (base == metadata) { + LOGGER.info("Nothing to commit."); + return; + } + + long start = System.currentTimeMillis(); + doCommit(base, metadata); + deleteRemovedMetadataFiles(base, metadata); + requestRefresh(); + + LOGGER.info( + "Successfully committed to table {} in {} ms", + tableName(), + System.currentTimeMillis() - start); + } + @Override public FileIO io() { return tableFileIO; From a9dcaa1ac6b4db29c0b9ce9b8970cac29bf2ae59 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 14:06:11 -0800 Subject: [PATCH 37/40] fix current metadata check --- .../service/catalog/BasePolarisCatalog.java | 76 ++++++++++++++++++- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index eaccc1d31..f60970315 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -36,10 +36,14 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; + +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.iceberg.BaseMetastoreTableOperations; import org.apache.iceberg.BaseTable; @@ -49,6 +53,7 @@ import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableMetadataParser; import org.apache.iceberg.TableOperations; +import org.apache.iceberg.TableProperties; import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.SupportsNamespaces; @@ -67,7 +72,9 @@ import org.apache.iceberg.io.CloseableGroup; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.SupportsBulkOperations; import org.apache.iceberg.util.PropertyUtil; +import org.apache.iceberg.util.Tasks; import org.apache.iceberg.view.BaseMetastoreViewCatalog; import org.apache.iceberg.view.BaseViewOperations; import org.apache.iceberg.view.ViewBuilder; @@ -1226,6 +1233,8 @@ private class BasePolarisTableOperations extends BaseMetastoreTableOperations { private final String fullTableName; private FileIO tableFileIO; + private ReentrantLock currentMetadataLock = new ReentrantLock(); + BasePolarisTableOperations(FileIO defaultFileIO, TableIdentifier tableIdentifier) { LOGGER.debug("new BasePolarisTableOperations for {}", tableIdentifier); this.tableIdentifier = tableIdentifier; @@ -1426,18 +1435,31 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { } } + /** + * COPIED FROM {@link BaseMetastoreTableOperations} but without the requirement that base == current() + * + * @param base table metadata on which changes were based + * @param metadata new table metadata with updates + */ @Override public void commit(TableMetadata base, TableMetadata metadata) { + TableMetadata currentMetadata = current(); + // if the metadata is already out of date, reject it - if (base.metadataFileLocation() != current().metadataFileLocation()) { - if (base != null) { - throw new CommitFailedException("Cannot commit: stale table metadata"); - } else { + if (base == null) { + if (currentMetadata != null) { // when current is non-null, the table exists. but when base is null, the commit is trying // to create the table throw new AlreadyExistsException("Table already exists: %s", tableName()); } + } else if (base.metadataFileLocation() != null && + !base.metadataFileLocation().equals(currentMetadata.metadataFileLocation())) { + throw new CommitFailedException("Cannot commit: stale table metadata"); + } else if (base != currentMetadata) { + // This branch is different from BaseMetastoreTableOperations + LOGGER.debug("Base object differs from current metadata; proceeding because locations match"); } + // if the metadata is not changed, return early if (base == metadata) { LOGGER.info("Nothing to commit."); @@ -1455,6 +1477,52 @@ public void commit(TableMetadata base, TableMetadata metadata) { System.currentTimeMillis() - start); } + /** + * + * COPIED FROM {@link BaseMetastoreTableOperations} + * + * Deletes the oldest metadata files if {@link + * TableProperties#METADATA_DELETE_AFTER_COMMIT_ENABLED} is true. + * + * @param base table metadata on which previous versions were based + * @param metadata new table metadata with updated previous versions + */ + protected void deleteRemovedMetadataFiles(TableMetadata base, TableMetadata metadata) { + if (base == null) { + return; + } + + boolean deleteAfterCommit = + metadata.propertyAsBoolean( + TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, + TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED_DEFAULT); + + if (deleteAfterCommit) { + Set removedPreviousMetadataFiles = + Sets.newHashSet(base.previousFiles()); + // TableMetadata#addPreviousFile builds up the metadata log and uses + // TableProperties.METADATA_PREVIOUS_VERSIONS_MAX to determine how many files should stay in + // the log, thus we don't include metadata.previousFiles() for deletion - everything else can + // be removed + removedPreviousMetadataFiles.removeAll(metadata.previousFiles()); + if (io() instanceof SupportsBulkOperations) { + ((SupportsBulkOperations) io()) + .deleteFiles( + Iterables.transform( + removedPreviousMetadataFiles, TableMetadata.MetadataLogEntry::file)); + } else { + Tasks.foreach(removedPreviousMetadataFiles) + .noRetry() + .suppressFailureWhenFinished() + .onFailure( + (previousMetadataFile, exc) -> + LOGGER.warn( + "Delete failed for previous metadata file: {}", previousMetadataFile, exc)) + .run(previousMetadataFile -> io().deleteFile(previousMetadataFile.file())); + } + } + } + @Override public FileIO io() { return tableFileIO; From 9b66016c5aef0e9c31b39b4c4249cd3ebf1460f2 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 14:06:14 -0800 Subject: [PATCH 38/40] autolint --- .../service/catalog/BasePolarisCatalog.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index f60970315..0276602de 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -23,6 +23,8 @@ import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import java.io.Closeable; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -41,9 +43,6 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; - -import com.google.common.collect.Iterables; -import com.google.common.collect.Sets; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.iceberg.BaseMetastoreTableOperations; import org.apache.iceberg.BaseTable; @@ -1436,7 +1435,8 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { } /** - * COPIED FROM {@link BaseMetastoreTableOperations} but without the requirement that base == current() + * COPIED FROM {@link BaseMetastoreTableOperations} but without the requirement that base == + * current() * * @param base table metadata on which changes were based * @param metadata new table metadata with updates @@ -1452,12 +1452,13 @@ public void commit(TableMetadata base, TableMetadata metadata) { // to create the table throw new AlreadyExistsException("Table already exists: %s", tableName()); } - } else if (base.metadataFileLocation() != null && - !base.metadataFileLocation().equals(currentMetadata.metadataFileLocation())) { + } else if (base.metadataFileLocation() != null + && !base.metadataFileLocation().equals(currentMetadata.metadataFileLocation())) { throw new CommitFailedException("Cannot commit: stale table metadata"); } else if (base != currentMetadata) { // This branch is different from BaseMetastoreTableOperations - LOGGER.debug("Base object differs from current metadata; proceeding because locations match"); + LOGGER.debug( + "Base object differs from current metadata; proceeding because locations match"); } // if the metadata is not changed, return early @@ -1478,10 +1479,9 @@ public void commit(TableMetadata base, TableMetadata metadata) { } /** - * * COPIED FROM {@link BaseMetastoreTableOperations} * - * Deletes the oldest metadata files if {@link + *

Deletes the oldest metadata files if {@link * TableProperties#METADATA_DELETE_AFTER_COMMIT_ENABLED} is true. * * @param base table metadata on which previous versions were based @@ -1502,7 +1502,8 @@ protected void deleteRemovedMetadataFiles(TableMetadata base, TableMetadata meta Sets.newHashSet(base.previousFiles()); // TableMetadata#addPreviousFile builds up the metadata log and uses // TableProperties.METADATA_PREVIOUS_VERSIONS_MAX to determine how many files should stay in - // the log, thus we don't include metadata.previousFiles() for deletion - everything else can + // the log, thus we don't include metadata.previousFiles() for deletion - everything else + // can // be removed removedPreviousMetadataFiles.removeAll(metadata.previousFiles()); if (io() instanceof SupportsBulkOperations) { @@ -1517,7 +1518,9 @@ protected void deleteRemovedMetadataFiles(TableMetadata base, TableMetadata meta .onFailure( (previousMetadataFile, exc) -> LOGGER.warn( - "Delete failed for previous metadata file: {}", previousMetadataFile, exc)) + "Delete failed for previous metadata file: {}", + previousMetadataFile, + exc)) .run(previousMetadataFile -> io().deleteFile(previousMetadataFile.file())); } } From 52b82f77dc93fbc6a9749abc00d46416c480fd92 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 14:08:03 -0800 Subject: [PATCH 39/40] fix --- .../polaris/service/catalog/PolarisCatalogHandlerWrapper.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 0bf2b60cc..25623c20c 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -1046,9 +1046,7 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest) TableOperations tableOps = ((BaseTable) table).operations(); final TableMetadata currentMetadata; if (baseCatalog instanceof BasePolarisCatalog basePolarisCatalog) { - // TODO use cached metadata once the check in BaseMetastoreTableOperations is fixed - // currentMetadata = basePolarisCatalog.loadTableMetadata(change.identifier()); - currentMetadata = tableOps.current(); + currentMetadata = basePolarisCatalog.loadTableMetadata(change.identifier()); } else { currentMetadata = tableOps.current(); } From ee3f401b6a0bb051c53d7a30ed04439bafa5e62a Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 14 Nov 2024 18:08:09 -0800 Subject: [PATCH 40/40] one change per review --- .../apache/polaris/service/catalog/BasePolarisCatalog.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java index 0276602de..a72886739 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java @@ -1459,10 +1459,8 @@ public void commit(TableMetadata base, TableMetadata metadata) { // This branch is different from BaseMetastoreTableOperations LOGGER.debug( "Base object differs from current metadata; proceeding because locations match"); - } - - // if the metadata is not changed, return early - if (base == metadata) { + } else if (base.metadataFileLocation().equals(metadata.metadataFileLocation())) { + // if the metadata is not changed, return early LOGGER.info("Nothing to commit."); return; }