diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java index cc8bae454d..f59a030059 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java @@ -190,4 +190,14 @@ protected FeatureConfiguration( .description("If true, the generic-tables endpoints are enabled") .defaultValue(false) .buildFeatureConfiguration(); + + public static final FeatureConfiguration DETAILED_PERSISTENCE_EXCEPTIONS = + PolarisConfiguration.builder() + .key("DETAILED_PERSISTENCE_EXCEPTIONS") + .description( + "If true, APIs like dropTable and dropGenericTable will report more detailed exceptions" + + " when a call to persistence fails. These exceptions may include corrected error codes and may" + + " result in improved retry behavior.") + .defaultValue(true) + .buildFeatureConfiguration(); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java index a4eee22cc4..6a29a8ab71 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java @@ -22,9 +22,22 @@ import com.fasterxml.jackson.annotation.JsonProperty; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; +import java.util.Optional; +import org.apache.iceberg.exceptions.AlreadyExistsException; +import org.apache.iceberg.exceptions.BadRequestException; +import org.apache.iceberg.exceptions.CommitFailedException; +import org.apache.iceberg.exceptions.NamespaceNotEmptyException; +import org.apache.iceberg.exceptions.NotFoundException; +import org.apache.iceberg.exceptions.UnprocessableEntityException; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.context.CallContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Base result class for any call to the persistence layer */ public class BaseResult { + private static final Logger LOGGER = LoggerFactory.getLogger(BaseResult.class); + // return code, indicates success or failure private final int returnStatusCode; @@ -57,14 +70,90 @@ public String getExtraInformation() { return extraInformation; } - public boolean isSuccess() { + public final boolean isSuccess() { return this.returnStatusCode == ReturnStatus.SUCCESS.getCode(); } - public boolean alreadyExists() { + public final boolean alreadyExists() { return this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode(); } + /** + * If this result is not a successful one, this builds an exception from the failed result which + * the exception mapper can use to provide the caller some useful information about the failure. + * The message relies on `extraInformation`. If {@link + * org.apache.polaris.core.config.FeatureConfiguration#DETAILED_PERSISTENCE_EXCEPTIONS} is false, + * this should return an empty option. + */ + @SuppressWarnings("FormatStringAnnotation") + public Optional getException(CallContext callContext) { + if (!callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getPolarisCallContext(), + FeatureConfiguration.DETAILED_PERSISTENCE_EXCEPTIONS)) { + return Optional.empty(); + } + + String message = this.extraInformation; + if (this.extraInformation == null) { + // TODO this should ideally never be hit but it's hit often. + // We should raise the logging level once it's less common + LOGGER.debug( + "A {} was discovered with status {} but without a detailed message", + this.getClass().getName(), + ReturnStatus.getStatus(this.returnStatusCode).name()); + message = "Error reported by the metastore"; + } + // TODO use a switch expression if the Java version here is ever raised + if (this.returnStatusCode == ReturnStatus.SUCCESS.getCode()) { + return Optional.empty(); + } else if (this.returnStatusCode == ReturnStatus.UNEXPECTED_ERROR_SIGNALED.getCode()) { + return Optional.of(new RuntimeException(message)); + } else if (this.returnStatusCode == ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED.getCode()) { + return Optional.of(new NotFoundException(message)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RESOLVED.getCode()) { + return Optional.of(new NotFoundException(message)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(message)); + } else if (this.returnStatusCode == ReturnStatus.GRANT_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(message)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode()) { + return Optional.of(new AlreadyExistsException(message)); + } else if (this.returnStatusCode == ReturnStatus.NAMESPACE_NOT_EMPTY.getCode()) { + return Optional.of(new NamespaceNotEmptyException(message)); + } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) { + return Optional.of(new BadRequestException(message)); + } else if (this.returnStatusCode + == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) { + return Optional.of(new CommitFailedException(message)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) { + return Optional.of(new BadRequestException(message)); + } else if (this.returnStatusCode == ReturnStatus.SUBSCOPE_CREDS_ERROR.getCode()) { + return Optional.of(new UnprocessableEntityException(message)); + } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(message)); + } else if (this.returnStatusCode + == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) { + return Optional.of(new AlreadyExistsException(message)); + } else { + return Optional.of(new RuntimeException(message)); + } + } + + /** + * If this result is failed, this should throw the appropriate exception which corresponds to the + * result status. See {@link BaseResult#getException} for details. If {@link + * org.apache.polaris.core.config.FeatureConfiguration#DETAILED_PERSISTENCE_EXCEPTIONS} is false, + * nothing should be thrown. + */ + public void maybeThrowException(CallContext callContext) throws RuntimeException { + if (this.getException(callContext).isPresent()) { + throw this.getException(callContext).get(); + } + } + /** Possible return code for the various API calls. */ public enum ReturnStatus { // all good diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java index ddcdf075d3..cf78c91141 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java @@ -41,6 +41,7 @@ import org.apache.iceberg.Schema; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.types.Types; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; @@ -546,7 +547,8 @@ public void testDropViaIceberg() { genericTableCatalog.createGenericTable( TableIdentifier.of("ns", "t1"), "format", "doc", Map.of()); - Assertions.assertThat(icebergCatalog.dropTable(TableIdentifier.of("ns", "t1"))).isFalse(); + Assertions.assertThatCode(() -> icebergCatalog.dropTable(TableIdentifier.of("ns", "t1"))) + .isInstanceOf(NotFoundException.class); Assertions.assertThat(genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1"))) .isNotNull(); } @@ -558,7 +560,8 @@ public void testDropViaIcebergView() { genericTableCatalog.createGenericTable( TableIdentifier.of("ns", "t1"), "format", "doc", Map.of()); - Assertions.assertThat(icebergCatalog.dropView(TableIdentifier.of("ns", "t1"))).isFalse(); + Assertions.assertThatCode(() -> icebergCatalog.dropView(TableIdentifier.of("ns", "t1"))) + .isInstanceOf(NotFoundException.class); Assertions.assertThat(genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1"))) .isNotNull(); } diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 4a5506fa22..06e76938b5 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -154,7 +154,9 @@ public Map getConfigOverrides() { "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"", "true", "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"", - "[\"FILE\"]"); + "[\"FILE\"]", + "polaris.features.defaults.\"DETAILED_PERSISTENCE_EXCEPTIONS\"", + "false"); } } diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java index 7d817bec4f..4c5a267af0 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java @@ -90,7 +90,9 @@ public Map getConfigOverrides() { "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"", "true", "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"", - "[\"FILE\"]"); + "[\"FILE\"]", + "polaris.features.defaults.\"DETAILED_PERSISTENCE_EXCEPTIONS\"", + "false"); } } diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java index effcededd4..83b0e04cb7 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java @@ -603,18 +603,7 @@ public void deleteCatalog(String name) { metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), cleanup); - // at least some handling of error - if (!dropEntityResult.isSuccess()) { - if (dropEntityResult.failedBecauseNotEmpty()) { - throw new BadRequestException( - "Catalog '%s' cannot be dropped, it is not empty", entity.getName()); - } else { - throw new BadRequestException( - "Catalog '%s' cannot be dropped, concurrent modification detected. Please try " - + "again", - entity.getName()); - } - } + dropEntityResult.maybeThrowException(callContext); } public @Nonnull CatalogEntity getCatalog(String name) { @@ -804,16 +793,7 @@ public void deletePrincipal(String name) { metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), false); - // at least some handling of error - if (!dropEntityResult.isSuccess()) { - if (dropEntityResult.isEntityUnDroppable()) { - throw new BadRequestException("Root principal cannot be dropped"); - } else { - throw new BadRequestException( - "Root principal cannot be dropped, concurrent modification " - + "detected. Please try again"); - } - } + dropEntityResult.maybeThrowException(callContext); } public @Nonnull PrincipalEntity getPrincipal(String name) { @@ -963,21 +943,11 @@ public void deletePrincipalRole(String name) { PolarisEntity entity = findPrincipalRoleByName(name) .orElseThrow(() -> new NotFoundException("PrincipalRole %s not found", name)); - // TODO: Handle return value in case of concurrent modification DropEntityResult dropEntityResult = metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), true); // cleanup grants - // at least some handling of error - if (!dropEntityResult.isSuccess()) { - if (dropEntityResult.isEntityUnDroppable()) { - throw new BadRequestException("Polaris service admin principal role cannot be dropped"); - } else { - throw new BadRequestException( - "Polaris service admin principal role cannot be dropped, " - + "concurrent modification detected. Please try again"); - } - } + dropEntityResult.maybeThrowException(callContext); } public @Nonnull PrincipalRoleEntity getPrincipalRole(String name) { @@ -1088,16 +1058,7 @@ public void deleteCatalogRole(String catalogName, String name) { Map.of(), true); // cleanup grants - // at least some handling of error - if (!dropEntityResult.isSuccess()) { - if (dropEntityResult.isEntityUnDroppable()) { - throw new BadRequestException("Catalog admin role cannot be dropped"); - } else { - throw new BadRequestException( - "Catalog admin role cannot be dropped, concurrent " - + "modification detected. Please try again"); - } - } + dropEntityResult.maybeThrowException(callContext); } public @Nonnull CatalogRoleEntity getCatalogRole(String catalogName, String name) { diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalog.java index b2fb31f67d..a3b104a2d1 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalog.java @@ -158,6 +158,7 @@ public boolean dropGenericTable(TableIdentifier tableIdentifier) { Map.of(), false); + dropEntityResult.maybeThrowException(callContext); return dropEntityResult.isSuccess(); } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 3f9722f793..fcea90132c 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -445,9 +445,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { DropEntityResult dropEntityResult = dropTableLike( PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, storageProperties, purge); - if (!dropEntityResult.isSuccess()) { - return false; - } + dropEntityResult.maybeThrowException(callContext); if (purge && lastMetadata != null && dropEntityResult.getCleanupTaskId() != null) { LOGGER.info( @@ -457,7 +455,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { taskExecutor.addTaskHandlerContext(dropEntityResult.getCleanupTaskId(), callContext); } - return true; + return dropEntityResult.isSuccess(); } @Override @@ -663,8 +661,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept if (!dropEntityResult.isSuccess() && dropEntityResult.failedBecauseNotEmpty()) { throw new NamespaceNotEmptyException("Namespace %s is not empty", namespace); } - - // return status of drop operation + dropEntityResult.maybeThrowException(callContext); return dropEntityResult.isSuccess(); } @@ -813,7 +810,10 @@ protected ViewOperations newViewOps(TableIdentifier identifier) { @Override public boolean dropView(TableIdentifier identifier) { - return dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(), true).isSuccess(); + DropEntityResult dropEntityResult = + dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(), true); + dropEntityResult.maybeThrowException(callContext); + return dropEntityResult.isSuccess(); } @Override @@ -1698,6 +1698,9 @@ private void renameTableLike( returnedEntityResult.getReturnStatus(), from, to); + + RuntimeException returnedEntityException = + returnedEntityResult.getException(callContext).orElse(new RuntimeException()); switch (returnedEntityResult.getReturnStatus()) { case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: { @@ -1706,34 +1709,37 @@ private void renameTableLike( if (existingEntitySubType == null) { // this code path is unexpected throw new AlreadyExistsException( - "Cannot rename %s to %s. Object already exists", from, to); + returnedEntityException, + "Cannot rename %s to %s. Object already exists", + from, + to); } else if (existingEntitySubType == PolarisEntitySubType.ICEBERG_TABLE) { throw new AlreadyExistsException( - "Cannot rename %s to %s. Table already exists", from, to); + returnedEntityException, + "Cannot rename %s to %s. Table already exists", + from, + to); } else if (existingEntitySubType == PolarisEntitySubType.ICEBERG_VIEW) { throw new AlreadyExistsException( - "Cannot rename %s to %s. View already exists", from, to); + returnedEntityException, "Cannot rename %s to %s. View already exists", from, to); } throw new IllegalStateException( - String.format("Unexpected entity type '%s'", existingEntitySubType)); + String.format("Unexpected entity type '%s'", existingEntitySubType), + returnedEntityException); } case BaseResult.ReturnStatus.ENTITY_NOT_FOUND: - throw new NotFoundException("Cannot rename %s to %s. %s does not exist", from, to, from); - - // this is temporary. Should throw a special error that will be caught and retried - case BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED: - case BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED: - throw new RuntimeException("concurrent update detected, please retry"); + throw new NotFoundException( + returnedEntityException, "Cannot rename %s to %s. %s does not exist", from, to, from); // some entities cannot be renamed case BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RENAMED: - throw new BadRequestException("Cannot rename built-in object %s", leafEntity.getName()); + throw new BadRequestException( + returnedEntityException, "Cannot rename built-in object %s", leafEntity.getName()); // some entities cannot be renamed default: - throw new IllegalStateException( - "Unknown error status " + returnedEntityResult.getReturnStatus()); + throw returnedEntityException; } } else { IcebergTableLikeEntity returnedEntity = @@ -1796,18 +1802,19 @@ private void createTableLike( .createEntityIfNotExists( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); if (!res.isSuccess()) { + RuntimeException resultException = + res.getException(callContext).orElse(new RuntimeException()); switch (res.getReturnStatus()) { case BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED: - throw new NotFoundException("Parent path does not exist for %s", identifier); - + throw new NotFoundException( + resultException, "Parent path does not exist for %s", identifier); case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: throw new AlreadyExistsException( - "Iceberg table, view, or generic table already exists: %s", identifier); + resultException, + "Iceberg table, view, or generic table already exists: %s", + identifier); default: - throw new IllegalStateException( - String.format( - "Unknown error status for identifier %s: %s with extraInfo: %s", - identifier, res.getReturnStatus(), res.getExtraInformation())); + throw resultException; } } PolarisEntity resultEntity = PolarisEntity.of(res); @@ -1833,19 +1840,19 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { .updateEntityPropertiesIfNotChanged( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); if (!res.isSuccess()) { + RuntimeException resultException = + res.getException(callContext).orElse(new RuntimeException()); switch (res.getReturnStatus()) { case BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED: - throw new NotFoundException("Parent path does not exist for %s", identifier); - + throw new NotFoundException( + resultException, "Parent path does not exist for %s", identifier); case BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED: throw new CommitFailedException( - "Failed to commit Table or View %s because it was concurrently modified", identifier); - + resultException, + "Failed to commit Table or View %s because it was concurrently modified", + identifier); default: - throw new IllegalStateException( - String.format( - "Unknown error status for identifier %s: %s with extraInfo: %s", - identifier, res.getReturnStatus(), res.getExtraInformation())); + throw resultException; } } PolarisEntity resultEntity = PolarisEntity.of(res); @@ -1911,9 +1918,11 @@ private boolean sendNotificationForTableLike( Preconditions.checkNotNull(notificationType, "Expected a valid notification type."); if (notificationType == NotificationType.DROP) { - return dropTableLike( - PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, Map.of(), false /* purge */) - .isSuccess(); + DropEntityResult dropEntityResult = + dropTableLike( + PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, Map.of(), false /* purge */); + dropEntityResult.maybeThrowException(callContext); + return dropEntityResult.isSuccess(); } else if (notificationType == NotificationType.VALIDATE) { // In this mode we don't want to make any mutations, so we won't auto-create non-existing // parent namespaces. This means when we want to validate allowedLocations for the proposed diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 718b4ef9e0..fd03a0997d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -260,14 +260,8 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) Map.of(), false); - if (!result.isSuccess()) { - throw new IllegalStateException( - String.format( - "Failed to drop policy %s error status: %s with extraInfo: %s", - policyIdentifier, result.getReturnStatus(), result.getExtraInformation())); - } - - return true; + result.maybeThrowException(callContext); + return result.isSuccess(); } public boolean attachPolicy(