Skip to content

Correctly Propagate Errors from Drop Entities #3693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
travis-bowen wants to merge 3 commits into
base: main
Choose a base branch
from
+189 −7
Open

Correctly Propagate Errors from Drop Entities #3693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3383939
Update error handling from drop entities to properly reflect what occ…
travis-bowen Feb 6, 2026
56a7f27
Update changelog.md
travis-bowen Feb 6, 2026
4b2c10c
PR Comments
travis-bowen Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti

### Fixes

- Fixed error propagation in drop operations (`dropTable`, `dropView`, `dropNamespace`). Server errors now return appropriate HTTP status codes based on persistence result instead of always returning NotFound
- Enable non-AWS STS role ARNs

### Commits
Expand Down
60 changes: 53 additions & 7 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,22 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) {
dropTableLike(
PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier, storageProperties, purge);
if (!dropEntityResult.isSuccess()) {
return false;
switch (dropEntityResult.getReturnStatus()) {
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
return false;

case BaseResult.ReturnStatus.ENTITY_UNDROPPABLE:
throw new ForbiddenException(
Copy link
Contributor

@HonahX HonahX Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this for table/view? I remember this is for service_admin and catalog_admin role which are not droppable.

Copy link
Contributor Author

@travis-bowen travis-bowen Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - we don't need it. I lean towards keeping it since we can't be certain of any custom implementations but I'm not tied to keeping it if you feel strongly.

Copy link
Contributor

@HonahX HonahX Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok to keep it. I feel like we probably should not group all the returnStatus in a single place.. but as Yufei mentioned that's a follow-up thing

"Table %s cannot be dropped: %s",
tableIdentifier, dropEntityResult.getExtraInformation());

default:
throw new ServiceFailureException(
"Failed to drop table %s, status=%s, extraInfo=%s",
tableIdentifier,
dropEntityResult.getReturnStatus(),
dropEntityResult.getExtraInformation());
}
}

if (purge && lastMetadata != null && dropEntityResult.getCleanupTaskId() != null) {
Expand Down Expand Up @@ -648,12 +663,25 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
Map.of(),
realmConfig.getConfig(FeatureConfiguration.CLEANUP_ON_NAMESPACE_DROP));

if (!dropEntityResult.isSuccess() && dropEntityResult.failedBecauseNotEmpty()) {
throw new NamespaceNotEmptyException("Namespace %s is not empty", namespace);
if (!dropEntityResult.isSuccess()) {
switch (dropEntityResult.getReturnStatus()) {
case BaseResult.ReturnStatus.NAMESPACE_NOT_EMPTY:
case BaseResult.ReturnStatus.CATALOG_NOT_EMPTY:
throw new NamespaceNotEmptyException("Namespace %s is not empty", namespace);

case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
return false;

default:
throw new ServiceFailureException(
"Failed to drop namespace %s, status=%s, extraInfo=%s",
namespace,
dropEntityResult.getReturnStatus(),
dropEntityResult.getExtraInformation());
}
}

// return status of drop operation
return dropEntityResult.isSuccess();
return true;
}

@Override
Expand Down Expand Up @@ -835,8 +863,26 @@ public boolean dropView(TableIdentifier identifier) {
boolean purge =
realmConfig.getConfig(FeatureConfiguration.PURGE_VIEW_METADATA_ON_DROP, catalogEntity);

return dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(), purge)
.isSuccess();
DropEntityResult dropEntityResult =
dropTableLike(PolarisEntitySubType.ICEBERG_VIEW, identifier, Map.of(), purge);
if (!dropEntityResult.isSuccess()) {
switch (dropEntityResult.getReturnStatus()) {
case BaseResult.ReturnStatus.ENTITY_NOT_FOUND:
return false;

case BaseResult.ReturnStatus.ENTITY_UNDROPPABLE:
throw new ForbiddenException(
"View %s cannot be dropped: %s", identifier, dropEntityResult.getExtraInformation());

default:
throw new ServiceFailureException(
"Failed to drop view %s, status=%s, extraInfo=%s",
identifier,
dropEntityResult.getReturnStatus(),
dropEntityResult.getExtraInformation());
}
}
return true;
}

@Override
Expand Down
135 changes: 135 additions & 0 deletions .../src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Fail.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
Expand Down Expand Up @@ -83,6 +86,7 @@
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
import org.apache.iceberg.exceptions.ServiceFailureException;
import org.apache.iceberg.inmemory.InMemoryFileIO;
import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.io.FileIO;
Expand Down Expand Up @@ -116,6 +120,7 @@
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
import org.apache.polaris.core.persistence.dao.entity.DropEntityResult;
import org.apache.polaris.core.persistence.dao.entity.EntityResult;
import org.apache.polaris.core.persistence.pagination.Page;
import org.apache.polaris.core.persistence.pagination.PageToken;
Expand Down Expand Up @@ -1962,6 +1967,136 @@ public void testDropTableWithPurgeDisabled() {
.hasMessageContaining(FeatureConfiguration.DROP_WITH_PURGE_ENABLED.key());
}

@Test
public void testDropNamespaceWithUnexpectedError() {
catalog.createNamespace(NS);

// Create a spy of the metaStoreManager to simulate an unexpected error
PolarisMetaStoreManager spiedManager = spy(metaStoreManager);
doReturn(
new DropEntityResult(
BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Simulated server error"))
.when(spiedManager)
.dropEntityIfExists(any(), anyList(), any(), anyMap(), anyBoolean());

IcebergCatalog spiedCatalog = newIcebergCatalog(CATALOG_NAME, spiedManager, fileIOFactory);
spiedCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO"));

Assertions.assertThatThrownBy(() -> spiedCatalog.dropNamespace(NS))
.isInstanceOf(ServiceFailureException.class)
.hasMessageContaining("Failed to drop namespace")
.hasMessageContaining("UNEXPECTED_ERROR_SIGNALED");
}

@Test
public void testDropTableWithUnexpectedError() {
catalog.createNamespace(NS);
catalog.buildTable(TABLE, SCHEMA).create();

// Create a spy of the metaStoreManager to simulate an unexpected error
PolarisMetaStoreManager spiedManager = spy(metaStoreManager);
doReturn(
new DropEntityResult(
BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Simulated server error"))
.when(spiedManager)
.dropEntityIfExists(any(), anyList(), any(), anyMap(), anyBoolean());

IcebergCatalog spiedCatalog = newIcebergCatalog(CATALOG_NAME, spiedManager, fileIOFactory);
spiedCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO"));

Assertions.assertThatThrownBy(() -> spiedCatalog.dropTable(TABLE, false))
.isInstanceOf(ServiceFailureException.class)
.hasMessageContaining("Failed to drop table")
.hasMessageContaining("UNEXPECTED_ERROR_SIGNALED");
}

@Test
public void testDropTableWithUndroppableEntity() {
catalog.createNamespace(NS);
catalog.buildTable(TABLE, SCHEMA).create();

// Create a spy of the metaStoreManager to simulate an undroppable entity error
PolarisMetaStoreManager spiedManager = spy(metaStoreManager);
doReturn(
new DropEntityResult(BaseResult.ReturnStatus.ENTITY_UNDROPPABLE, "Entity is protected"))
.when(spiedManager)
.dropEntityIfExists(any(), anyList(), any(), anyMap(), anyBoolean());

IcebergCatalog spiedCatalog = newIcebergCatalog(CATALOG_NAME, spiedManager, fileIOFactory);
spiedCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO"));

Assertions.assertThatThrownBy(() -> spiedCatalog.dropTable(TABLE, false))
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("cannot be dropped");
}

@Test
public void testDropViewWithUnexpectedError() {
catalog.createNamespace(NS);
catalog
.buildView(TABLE)
.withSchema(SCHEMA)
.withDefaultNamespace(NS)
.withQuery("spark", "SELECT * FROM ns.tbl")
.create();

// Create a spy of the metaStoreManager to simulate an unexpected error
PolarisMetaStoreManager spiedManager = spy(metaStoreManager);
doReturn(
new DropEntityResult(
BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Simulated server error"))
.when(spiedManager)
.dropEntityIfExists(any(), anyList(), any(), anyMap(), anyBoolean());

IcebergCatalog spiedCatalog = newIcebergCatalog(CATALOG_NAME, spiedManager, fileIOFactory);
spiedCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO"));

Assertions.assertThatThrownBy(() -> spiedCatalog.dropView(TABLE))
.isInstanceOf(ServiceFailureException.class)
.hasMessageContaining("Failed to drop view")
.hasMessageContaining("UNEXPECTED_ERROR_SIGNALED");
}

@Test
public void testDropViewWithUndroppableEntity() {
catalog.createNamespace(NS);
catalog
.buildView(TABLE)
.withSchema(SCHEMA)
.withDefaultNamespace(NS)
.withQuery("spark", "SELECT * FROM ns.tbl")
.create();

// Create a spy of the metaStoreManager to simulate an undroppable entity error
PolarisMetaStoreManager spiedManager = spy(metaStoreManager);
doReturn(
new DropEntityResult(BaseResult.ReturnStatus.ENTITY_UNDROPPABLE, "Entity is protected"))
.when(spiedManager)
.dropEntityIfExists(any(), anyList(), any(), anyMap(), anyBoolean());

IcebergCatalog spiedCatalog = newIcebergCatalog(CATALOG_NAME, spiedManager, fileIOFactory);
spiedCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO"));

Assertions.assertThatThrownBy(() -> spiedCatalog.dropView(TABLE))
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("cannot be dropped");
}

private TableMetadata createSampleTableMetadata(String tableLocation) {
Schema schema =
new Schema(
Expand Down
Loading