From 37320081202a40cd71dfcb89ffcb08ea533edf44 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 14:18:56 -0700 Subject: [PATCH 1/6] partial changes --- .../persistence/dao/entity/BaseResult.java | 64 ++++++++++++++++++- .../service/admin/PolarisAdminService.java | 47 ++------------ .../catalog/generic/GenericTableCatalog.java | 1 + .../catalog/iceberg/IcebergCatalog.java | 27 ++++---- .../service/catalog/policy/PolicyCatalog.java | 1 + 5 files changed, 80 insertions(+), 60 deletions(-) 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..637d845998 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,6 +22,14 @@ import com.fasterxml.jackson.annotation.JsonProperty; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; +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 java.util.Optional; /** Base result class for any call to the persistence layer */ public class BaseResult { @@ -57,15 +65,65 @@ 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(); } - /** Possible return code for the various API calls. */ + /** + * 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`. + */ + public Optional getException() { + // 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(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED.getCode()) { + return Optional.of(new NotFoundException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RESOLVED.getCode()) { + return Optional.of(new NotFoundException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.GRANT_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode()) { + return Optional.of(new AlreadyExistsException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.NAMESPACE_NOT_EMPTY.getCode()) { + return Optional.of(new NamespaceNotEmptyException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) { + return Optional.of(new BadRequestException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) { + return Optional.of(new CommitFailedException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) { + return Optional.of(new BadRequestException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.SUBSCOPE_CREDS_ERROR.getCode()) { + return Optional.of(new UnprocessableEntityException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) { + return Optional.of(new NotFoundException(this.extraInformation)); + } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) { + return Optional.of(new AlreadyExistsException(this.extraInformation)); + } else { + return Optional.of(new RuntimeException(this.extraInformation)); + } + } + + /** + * If this result is failed, this should throw the appropriate exception which corresponds + * to the result status. See {@link BaseResult#getException} for details. + */ + public void maybeThrowException() throws RuntimeException { + if (this.getException().isPresent()) { + throw this.getException().get(); + } + } + + /** Possible return code for the various API calls. */ public enum ReturnStatus { // all good SUCCESS(1), 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..2a83091de2 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(); } 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(); } 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(); } 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(); } 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..151bbe6ba8 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(); 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..b52a8de009 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 @@ -1691,13 +1691,16 @@ private void renameTableLike( PolarisEntity.toCoreList(newCatalogPath), toEntity); + // handle error - if (!returnedEntityResult.isSuccess()) { + if (returnedEntityResult.getException().isPresent()) { LOGGER.debug( "Rename error {} trying to rename {} to {}. Checking existing object.", returnedEntityResult.getReturnStatus(), from, to); + + RuntimeException returnedEntityException = returnedEntityResult.getException().get(); switch (returnedEntityResult.getReturnStatus()) { case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: { @@ -1706,34 +1709,30 @@ 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 = 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 a4fb23da7d..f8cdb85625 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 @@ -269,6 +269,7 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) Map.of(), false); + dropEntityResult.maybeThrowException(); return dropEntityResult.isSuccess(); } From 7e1daeccc0608ed033de4eec2037519115a3bd99 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 14:23:50 -0700 Subject: [PATCH 2/6] more refactoring --- .../catalog/iceberg/IcebergCatalog.java | 48 ++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) 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 b52a8de009..81e1a27fe4 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(); if (purge && lastMetadata != null && dropEntityResult.getCleanupTaskId() != null) { LOGGER.info( @@ -663,9 +661,8 @@ 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 - return dropEntityResult.isSuccess(); + dropEntityResult.maybeThrowException(); + return true; } @Override @@ -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(); + return true; } @Override @@ -1794,19 +1794,16 @@ private void createTableLike( getMetaStoreManager() .createEntityIfNotExists( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); - if (!res.isSuccess()) { + if (res.getException().isPresent()) { + RuntimeException resultException = res.getException().get(); 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); @@ -1831,20 +1828,16 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { getMetaStoreManager() .updateEntityPropertiesIfNotChanged( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); - if (!res.isSuccess()) { + if (res.getException().isPresent()) { + RuntimeException resultException = res.getException().get(); 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); @@ -1910,9 +1903,10 @@ 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(); + return true; } 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 From 44ed16be8913fbaedea7e1f2c562280359485686 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 14:37:10 -0700 Subject: [PATCH 3/6] autolint --- .../persistence/dao/entity/BaseResult.java | 24 +++++++------- .../catalog/iceberg/IcebergCatalog.java | 31 +++++++++++++------ 2 files changed, 35 insertions(+), 20 deletions(-) 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 637d845998..cb9edd2a07 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,6 +22,7 @@ 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; @@ -29,8 +30,6 @@ import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.exceptions.UnprocessableEntityException; -import java.util.Optional; - /** Base result class for any call to the persistence layer */ public class BaseResult { // return code, indicates success or failure @@ -74,10 +73,11 @@ public final boolean alreadyExists() { } /** - * 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 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`. */ + @SuppressWarnings("FormatStringAnnotation") public Optional getException() { // TODO use a switch expression if the Java version here is ever raised if (this.returnStatusCode == ReturnStatus.SUCCESS.getCode()) { @@ -98,7 +98,8 @@ public Optional getException() { return Optional.of(new NamespaceNotEmptyException(this.extraInformation)); } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) { return Optional.of(new BadRequestException(this.extraInformation)); - } else if (this.returnStatusCode == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) { + } else if (this.returnStatusCode + == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) { return Optional.of(new CommitFailedException(this.extraInformation)); } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) { return Optional.of(new BadRequestException(this.extraInformation)); @@ -106,7 +107,8 @@ public Optional getException() { return Optional.of(new UnprocessableEntityException(this.extraInformation)); } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) { return Optional.of(new NotFoundException(this.extraInformation)); - } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) { + } else if (this.returnStatusCode + == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) { return Optional.of(new AlreadyExistsException(this.extraInformation)); } else { return Optional.of(new RuntimeException(this.extraInformation)); @@ -114,16 +116,16 @@ public Optional getException() { } /** - * If this result is failed, this should throw the appropriate exception which corresponds - * to the result status. See {@link BaseResult#getException} for details. + * If this result is failed, this should throw the appropriate exception which corresponds to the + * result status. See {@link BaseResult#getException} for details. */ public void maybeThrowException() throws RuntimeException { if (this.getException().isPresent()) { - throw this.getException().get(); + throw this.getException().get(); } } - /** Possible return code for the various API calls. */ + /** Possible return code for the various API calls. */ public enum ReturnStatus { // all good SUCCESS(1), 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 81e1a27fe4..8b968c4e9b 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 @@ -1691,7 +1691,6 @@ private void renameTableLike( PolarisEntity.toCoreList(newCatalogPath), toEntity); - // handle error if (returnedEntityResult.getException().isPresent()) { LOGGER.debug( @@ -1709,16 +1708,23 @@ private void renameTableLike( if (existingEntitySubType == null) { // this code path is unexpected throw new AlreadyExistsException( - returnedEntityException, "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( - returnedEntityException, "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( returnedEntityException, "Cannot rename %s to %s. View already exists", from, to); } throw new IllegalStateException( - String.format("Unexpected entity type '%s'", existingEntitySubType), returnedEntityException); + String.format("Unexpected entity type '%s'", existingEntitySubType), + returnedEntityException); } case BaseResult.ReturnStatus.ENTITY_NOT_FOUND: @@ -1798,10 +1804,13 @@ private void createTableLike( RuntimeException resultException = res.getException().get(); switch (res.getReturnStatus()) { case BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED: - throw new NotFoundException(resultException, "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( - resultException, "Iceberg table, view, or generic table already exists: %s", identifier); + resultException, + "Iceberg table, view, or generic table already exists: %s", + identifier); default: throw resultException; } @@ -1832,10 +1841,13 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { RuntimeException resultException = res.getException().get(); switch (res.getReturnStatus()) { case BaseResult.ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED: - throw new NotFoundException(resultException, "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( - resultException, "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 resultException; } @@ -1903,7 +1915,8 @@ private boolean sendNotificationForTableLike( Preconditions.checkNotNull(notificationType, "Expected a valid notification type."); if (notificationType == NotificationType.DROP) { - DropEntityResult dropEntityResult = dropTableLike( + DropEntityResult dropEntityResult = + dropTableLike( PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, Map.of(), false /* purge */); dropEntityResult.maybeThrowException(); return true; From 423c180e48a8112f8687872043c4aca1eb22c5ec Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 14:48:18 -0700 Subject: [PATCH 4/6] fix message length --- .../persistence/dao/entity/BaseResult.java | 41 ++++++++++++------- 1 file changed, 27 insertions(+), 14 deletions(-) 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 cb9edd2a07..03c534a4bc 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 @@ -29,9 +29,14 @@ import org.apache.iceberg.exceptions.NamespaceNotEmptyException; import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.exceptions.UnprocessableEntityException; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; +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; @@ -79,39 +84,47 @@ public final boolean alreadyExists() { */ @SuppressWarnings("FormatStringAnnotation") public Optional getException() { + String message = this.extraInformation; + if (this.extraInformation == null) { + LOGGER.warn( + "A {} was discovered with status {} but without a detailed message", + this.getClass().getName(), + ReturnStatus.getStatus(this.returnStatusCode).name()); + message = ""; + } // 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(this.extraInformation)); + return Optional.of(new RuntimeException(message)); } else if (this.returnStatusCode == ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED.getCode()) { - return Optional.of(new NotFoundException(this.extraInformation)); + return Optional.of(new NotFoundException(message)); } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RESOLVED.getCode()) { - return Optional.of(new NotFoundException(this.extraInformation)); + return Optional.of(new NotFoundException(message)); } else if (this.returnStatusCode == ReturnStatus.ENTITY_NOT_FOUND.getCode()) { - return Optional.of(new NotFoundException(this.extraInformation)); + return Optional.of(new NotFoundException(message)); } else if (this.returnStatusCode == ReturnStatus.GRANT_NOT_FOUND.getCode()) { - return Optional.of(new NotFoundException(this.extraInformation)); + return Optional.of(new NotFoundException(message)); } else if (this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode()) { - return Optional.of(new AlreadyExistsException(this.extraInformation)); + return Optional.of(new AlreadyExistsException(message)); } else if (this.returnStatusCode == ReturnStatus.NAMESPACE_NOT_EMPTY.getCode()) { - return Optional.of(new NamespaceNotEmptyException(this.extraInformation)); + return Optional.of(new NamespaceNotEmptyException(message)); } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) { - return Optional.of(new BadRequestException(this.extraInformation)); + return Optional.of(new BadRequestException(message)); } else if (this.returnStatusCode == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) { - return Optional.of(new CommitFailedException(this.extraInformation)); + return Optional.of(new CommitFailedException(message)); } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) { - return Optional.of(new BadRequestException(this.extraInformation)); + return Optional.of(new BadRequestException(message)); } else if (this.returnStatusCode == ReturnStatus.SUBSCOPE_CREDS_ERROR.getCode()) { - return Optional.of(new UnprocessableEntityException(this.extraInformation)); + return Optional.of(new UnprocessableEntityException(message)); } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) { - return Optional.of(new NotFoundException(this.extraInformation)); + return Optional.of(new NotFoundException(message)); } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) { - return Optional.of(new AlreadyExistsException(this.extraInformation)); + return Optional.of(new AlreadyExistsException(message)); } else { - return Optional.of(new RuntimeException(this.extraInformation)); + return Optional.of(new RuntimeException(message)); } } From 720102e0377fc04910def3e85e39b8bc63572dc8 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 15:29:56 -0700 Subject: [PATCH 5/6] feature flag --- .../core/config/FeatureConfiguration.java | 9 +++++ .../persistence/dao/entity/BaseResult.java | 34 ++++++++++++++----- .../quarkus/catalog/IcebergCatalogTest.java | 4 ++- .../service/admin/PolarisAdminService.java | 8 ++--- .../catalog/generic/GenericTableCatalog.java | 3 +- .../catalog/iceberg/IcebergCatalog.java | 29 ++++++++-------- .../service/catalog/policy/PolicyCatalog.java | 2 +- 7 files changed, 60 insertions(+), 29 deletions(-) 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..a7023fd762 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,13 @@ 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 03c534a4bc..9a4d95abe9 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 @@ -29,7 +29,9 @@ import org.apache.iceberg.exceptions.NamespaceNotEmptyException; import org.apache.iceberg.exceptions.NotFoundException; import org.apache.iceberg.exceptions.UnprocessableEntityException; -import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.config.PolarisConfiguration; +import org.apache.polaris.core.context.CallContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -80,13 +82,27 @@ public final boolean alreadyExists() { /** * 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`. + * 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() { + 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) { - LOGGER.warn( + // 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()); @@ -130,11 +146,13 @@ public Optional getException() { /** * If this result is failed, this should throw the appropriate exception which corresponds to the - * result status. See {@link BaseResult#getException} for details. + * 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() throws RuntimeException { - if (this.getException().isPresent()) { - throw this.getException().get(); + public void maybeThrowException(CallContext callContext) throws RuntimeException { + if (this.getException(callContext).isPresent()) { + throw this.getException(callContext).get(); } } 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/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 2a83091de2..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,7 +603,7 @@ public void deleteCatalog(String name) { metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), cleanup); - dropEntityResult.maybeThrowException(); + dropEntityResult.maybeThrowException(callContext); } public @Nonnull CatalogEntity getCatalog(String name) { @@ -793,7 +793,7 @@ public void deletePrincipal(String name) { metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), false); - dropEntityResult.maybeThrowException(); + dropEntityResult.maybeThrowException(callContext); } public @Nonnull PrincipalEntity getPrincipal(String name) { @@ -947,7 +947,7 @@ public void deletePrincipalRole(String name) { metaStoreManager.dropEntityIfExists( getCurrentPolarisContext(), null, entity, Map.of(), true); // cleanup grants - dropEntityResult.maybeThrowException(); + dropEntityResult.maybeThrowException(callContext); } public @Nonnull PrincipalRoleEntity getPrincipalRole(String name) { @@ -1058,7 +1058,7 @@ public void deleteCatalogRole(String catalogName, String name) { Map.of(), true); // cleanup grants - dropEntityResult.maybeThrowException(); + 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 151bbe6ba8..8c20be9fb5 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 @@ -26,6 +26,7 @@ import org.apache.iceberg.exceptions.NoSuchNamespaceException; import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.polaris.core.catalog.PolarisCatalogHelpers; +import org.apache.polaris.core.config.FeatureConfiguration; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntity; @@ -158,7 +159,7 @@ public boolean dropGenericTable(TableIdentifier tableIdentifier) { Map.of(), false); - dropEntityResult.maybeThrowException(); + 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 8b968c4e9b..adc06dea65 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,7 +445,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { DropEntityResult dropEntityResult = dropTableLike( PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, storageProperties, purge); - dropEntityResult.maybeThrowException(); + dropEntityResult.maybeThrowException(callContext); if (purge && lastMetadata != null && dropEntityResult.getCleanupTaskId() != null) { LOGGER.info( @@ -455,7 +455,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { taskExecutor.addTaskHandlerContext(dropEntityResult.getCleanupTaskId(), callContext); } - return true; + return dropEntityResult.isSuccess(); } @Override @@ -661,8 +661,8 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept if (!dropEntityResult.isSuccess() && dropEntityResult.failedBecauseNotEmpty()) { throw new NamespaceNotEmptyException("Namespace %s is not empty", namespace); } - dropEntityResult.maybeThrowException(); - return true; + dropEntityResult.maybeThrowException(callContext); + return dropEntityResult.isSuccess(); } @Override @@ -812,8 +812,8 @@ protected ViewOperations newViewOps(TableIdentifier identifier) { public boolean dropView(TableIdentifier identifier) { DropEntityResult dropEntityResult = dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(), true); - dropEntityResult.maybeThrowException(); - return true; + dropEntityResult.maybeThrowException(callContext); + return dropEntityResult.isSuccess(); } @Override @@ -1692,14 +1692,15 @@ private void renameTableLike( toEntity); // handle error - if (returnedEntityResult.getException().isPresent()) { + if (!returnedEntityResult.isSuccess()) { LOGGER.debug( "Rename error {} trying to rename {} to {}. Checking existing object.", returnedEntityResult.getReturnStatus(), from, to); - RuntimeException returnedEntityException = returnedEntityResult.getException().get(); + RuntimeException returnedEntityException = + returnedEntityResult.getException(callContext).orElse(new RuntimeException()); switch (returnedEntityResult.getReturnStatus()) { case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: { @@ -1800,8 +1801,8 @@ private void createTableLike( getMetaStoreManager() .createEntityIfNotExists( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); - if (res.getException().isPresent()) { - RuntimeException resultException = res.getException().get(); + 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( @@ -1837,8 +1838,8 @@ private void updateTableLike(TableIdentifier identifier, PolarisEntity entity) { getMetaStoreManager() .updateEntityPropertiesIfNotChanged( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); - if (res.getException().isPresent()) { - RuntimeException resultException = res.getException().get(); + 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( @@ -1918,8 +1919,8 @@ private boolean sendNotificationForTableLike( DropEntityResult dropEntityResult = dropTableLike( PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, Map.of(), false /* purge */); - dropEntityResult.maybeThrowException(); - return true; + 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 f8cdb85625..2ff48821ef 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 @@ -269,7 +269,7 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) Map.of(), false); - dropEntityResult.maybeThrowException(); + dropEntityResult.maybeThrowException(callContext); return dropEntityResult.isSuccess(); } From 80c35b67d36bffb9a2a4faf6a5bd22ada91b3cd6 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Wed, 9 Apr 2025 16:12:22 -0700 Subject: [PATCH 6/6] stable --- .../polaris/core/persistence/dao/entity/BaseResult.java | 2 +- .../service/quarkus/catalog/GenericTableCatalogTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 4666896155..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 @@ -104,7 +104,7 @@ public Optional getException(CallContext callContext) { "A {} was discovered with status {} but without a detailed message", this.getClass().getName(), ReturnStatus.getStatus(this.returnStatusCode).name()); - message = ""; + 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()) { 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 de8737fe84..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 @@ -28,7 +28,6 @@ import io.quarkus.test.junit.QuarkusTestProfile; import io.quarkus.test.junit.TestProfile; import jakarta.inject.Inject; -import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.SecurityContext; import java.io.IOException; import java.lang.reflect.Method; @@ -42,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; @@ -560,7 +560,7 @@ public void testDropViaIcebergView() { genericTableCatalog.createGenericTable( TableIdentifier.of("ns", "t1"), "format", "doc", Map.of()); - Assertions.assertThat(icebergCatalog.dropView(TableIdentifier.of("ns", "t1"))) + Assertions.assertThatCode(() -> icebergCatalog.dropView(TableIdentifier.of("ns", "t1"))) .isInstanceOf(NotFoundException.class); Assertions.assertThat(genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1"))) .isNotNull();