Skip to content

[#10737] fix(core): Avoid blocking dropCatalog on imported schemas #10738

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
diqiu50 wants to merge 7 commits into
base: main
Choose a base branch
from
+196 −3
Open

[#10737] fix(core): Avoid blocking dropCatalog on imported schemas #10738

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3af686d
[#10737] fix(core): Avoid blocking dropCatalog on imported schemas
diqiu50 Apr 10, 2026
c3e9723
style: apply spotless formatting
diqiu50 Apr 10, 2026
30e2dab
fix(core): Address Copilot review comments for drop-catalog schema cl…
diqiu50 Apr 13, 2026
b0088ca
refactor(core): Use constant for schema created-by Gravitino value
diqiu50 Apr 13, 2026
fd0a696
fix(core): Remove entity-store marker, rely solely on StringIdentifie…
diqiu50 Apr 16, 2026
f780666
fix(core): Handle backends without StringIdentifier support (e.g., My…
diqiu50 Apr 16, 2026
642c7e5
fix(core): Treat empty schema properties as user-created to fix MySQL…
diqiu50 Apr 17, 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
38 changes: 35 additions & 3 deletions core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.Schema;
import org.apache.gravitino.StringIdentifier;
import org.apache.gravitino.connector.BaseCatalog;
import org.apache.gravitino.connector.CatalogOperations;
Expand All @@ -87,6 +88,7 @@
import org.apache.gravitino.exceptions.NoSuchCatalogException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.exceptions.NonEmptyCatalogException;
import org.apache.gravitino.exceptions.NonEmptyEntityException;
import org.apache.gravitino.file.FilesetCatalog;
Expand Down Expand Up @@ -877,9 +879,39 @@ private boolean containsUserCreatedSchemas(
Set<String> availableSchemaNames =
Arrays.stream(allSchemas).map(NameIdentifier::name).collect(Collectors.toSet());

// some schemas are dropped externally, but still exist in the entity store, those schemas are
// invalid
return schemaEntities.stream().map(SchemaEntity::name).anyMatch(availableSchemaNames::contains);
// Some schemas are dropped externally, but still exist in the entity store — those are invalid.
// Among schemas that still exist in the underlying catalog, only count those created via
// Gravitino. New schemas carry an entity-store marker; older schemas fall back to the embedded
// StringIdentifier in external catalog metadata.
return schemaEntities.stream()
.filter(e -> availableSchemaNames.contains(e.name()))
.anyMatch(
e -> {
Map<String, String> entityProps = e.properties();
if (entityProps != null
&& "true"
.equals(entityProps.get(SchemaOperationDispatcher.SCHEMA_CREATED_BY_GRAVITINO))) {
return true;
}

try {
Schema schema =
catalogWrapper.doWithSchemaOps(ops -> ops.loadSchema(e.nameIdentifier()));
return StringIdentifier.fromProperties(schema.properties()) != null;
} catch (NoSuchSchemaException ex) {
LOG.warn(
"Schema {} no longer exists while checking whether it is user-created",
e.nameIdentifier(),
ex);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

This logs at WARN (with stack trace) when loadSchema throws NoSuchSchemaException, but this can happen due to a normal race between listSchemas and loadSchema (and you already filtered by availableSchemaNames). Consider lowering to DEBUG or logging without the exception to avoid noisy logs during catalog drop.

Suggested change
LOG.warn(
"Schema {} no longer exists while checking whether it is user-created",
e.nameIdentifier(),
ex);
LOG.debug(
"Schema {} no longer exists while checking whether it is user-created",
e.nameIdentifier());

Copilot uses AI. Check for mistakes.
return false;
} catch (Exception ex) {
throw new RuntimeException(
String.format(
"Failed to determine whether schema %s is user-created",
e.nameIdentifier()),
ex);
}
});
Comment thread
diqiu50 marked this conversation as resolved.
Outdated
}

/**
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.apache.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForCreate;
import static org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier;

import com.google.common.collect.ImmutableMap;
import java.time.Instant;
import java.util.Map;
import org.apache.gravitino.EntityAlreadyExistsException;
Expand Down Expand Up @@ -51,6 +52,8 @@

public class SchemaOperationDispatcher extends OperationDispatcher implements SchemaDispatcher {

static final String SCHEMA_CREATED_BY_GRAVITINO = "gravitino.created";

private static final Logger LOG = LoggerFactory.getLogger(SchemaOperationDispatcher.class);

/**
Expand Down Expand Up @@ -148,6 +151,7 @@ public Schema createSchema(NameIdentifier ident, String comment, Map<String, Str
.withId(uid)
.withName(ident.name())
.withNamespace(ident.namespace())
.withProperties(ImmutableMap.of(SCHEMA_CREATED_BY_GRAVITINO, "true"))
.withAuditInfo(
AuditInfo.builder()
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
Expand Down
92 changes: 92 additions & 0 deletions core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,14 @@
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.Schema;
import org.apache.gravitino.connector.capability.Capability;
import org.apache.gravitino.connector.capability.CapabilityResult;
import org.apache.gravitino.exceptions.CatalogAlreadyExistsException;
import org.apache.gravitino.exceptions.CatalogInUseException;
import org.apache.gravitino.exceptions.NoSuchCatalogException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.lock.LockManager;
import org.apache.gravitino.meta.AuditInfo;
import org.apache.gravitino.meta.BaseMetalake;
Expand Down Expand Up @@ -580,6 +582,96 @@ public void testDropCatalog() throws Exception {
boolean dropped = catalogManager.dropCatalog(ident);
Assertions.assertTrue(dropped);

Catalog catalogImported =
catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, comment, props);
Mockito.doCallRealMethod().when(catalogManager).loadCatalogAndWrap(ident);
Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
CatalogEntity importedCatalogEntity =
entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class);
FieldUtils.writeField(catalogImported, "entity", importedCatalogEntity, true);
SchemaEntity importedSchemaEntity =
SchemaEntity.builder()
.withId(RandomIdGenerator.INSTANCE.nextId())
.withName("imported_schema")
.withNamespace(Namespace.of("metalake", "test41"))
.withAuditInfo(
AuditInfo.builder()
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
entityStore.put(importedSchemaEntity);
Schema importedSchema = Mockito.mock(Schema.class);
Mockito.doReturn(ImmutableMap.of()).when(importedSchema).properties();
CatalogManager.CatalogWrapper importedCatalogWrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
Capability importedCapability = Mockito.mock(Capability.class);
Mockito.doReturn(importedCatalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
Mockito.doReturn(catalogImported).when(importedCatalogWrapper).catalog();
Mockito.doReturn(importedCapability).when(importedCatalogWrapper).capabilities();
Mockito.doReturn(unsupportedResult).when(importedCapability).managedStorage(any());
Mockito.doReturn(new NameIdentifier[] {NameIdentifier.of("metalake", "test41", "imported_schema")})
.doReturn(importedSchema)
.when(importedCatalogWrapper)
.doWithSchemaOps(any());
Assertions.assertTrue(catalogManager.dropCatalog(ident));

Catalog catalog2 =
catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, comment, props);
Mockito.doCallRealMethod().when(catalogManager).loadCatalogAndWrap(ident);
Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
CatalogEntity oldEntity2 = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class);
FieldUtils.writeField(catalog2, "entity", oldEntity2, true);
CatalogManager.CatalogWrapper missingSchemaCatalogWrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
Capability missingSchemaCapability = Mockito.mock(Capability.class);
Mockito.doReturn(missingSchemaCatalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
Mockito.doReturn(catalog2).when(missingSchemaCatalogWrapper).catalog();
Mockito.doReturn(missingSchemaCapability).when(missingSchemaCatalogWrapper).capabilities();
Mockito.doReturn(unsupportedResult).when(missingSchemaCapability).managedStorage(any());
Mockito.doReturn(new NameIdentifier[] {NameIdentifier.of("metalake", "test41", "default")})
.doThrow(new NoSuchSchemaException("Schema not found"))
.when(missingSchemaCatalogWrapper)
.doWithSchemaOps(any());
Assertions.assertTrue(catalogManager.dropCatalog(ident));

Comment thread
diqiu50 marked this conversation as resolved.
Outdated
Catalog catalog3 =
catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, comment, props);
Mockito.doCallRealMethod().when(catalogManager).loadCatalogAndWrap(ident);
Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
CatalogEntity oldEntity3 = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class);
FieldUtils.writeField(catalog3, "entity", oldEntity3, true);
SchemaEntity schemaEntity =
SchemaEntity.builder()
.withId(RandomIdGenerator.INSTANCE.nextId())
.withName("test_schema1")
.withNamespace(Namespace.of("metalake", "test41"))
.withAuditInfo(
AuditInfo.builder()
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
entityStore.put(schemaEntity);
CatalogManager.CatalogWrapper runtimeErrorCatalogWrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
Capability runtimeErrorCapability = Mockito.mock(Capability.class);
Mockito.doReturn(runtimeErrorCatalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
Mockito.doReturn(catalog3).when(runtimeErrorCatalogWrapper).catalog();
Mockito.doReturn(runtimeErrorCapability).when(runtimeErrorCatalogWrapper).capabilities();
Mockito.doReturn(unsupportedResult).when(runtimeErrorCapability).managedStorage(any());
Mockito.doReturn(new NameIdentifier[] {NameIdentifier.of("metalake", "test41", "test_schema1")})
.doThrow(new RuntimeException("Failed connect"))
.when(runtimeErrorCatalogWrapper)
.doWithSchemaOps(any());
RuntimeException runtimeException =
Assertions.assertThrows(RuntimeException.class, () -> catalogManager.dropCatalog(ident));
Assertions.assertTrue(
runtimeException
.getMessage()
.contains(
"Failed to determine whether schema metalake.test41.test_schema1 is user-created"));

// Test drop non-existed catalog
NameIdentifier ident1 = NameIdentifier.of("metalake", "test42");
boolean dropped1 = catalogManager.dropCatalog(ident1);
Expand Down
Loading