Copilot commented on code in PR #10738:
URL: https://github.com/apache/gravitino/pull/10738#discussion_r3063588907
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -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);
+ return false;
+ } catch (Exception ex) {
+ throw new RuntimeException(
+ String.format(
+ "Failed to determine whether schema %s is
user-created",
+ e.nameIdentifier()),
+ ex);
+ }
+ });
Review Comment:
`containsUserCreatedSchemas` now performs a per-schema `loadSchema` call
inside a stream/lambda. This makes the drop path potentially O(n) remote calls
and forces checked exceptions to be wrapped in a `RuntimeException`, which
obscures the original exception type at the call site. Consider rewriting this
block as an explicit loop (to allow checked exceptions to propagate) and
short-circuiting as soon as a user-created schema is detected; this will also
make the control flow and error handling clearer.
```suggestion
for (SchemaEntity schemaEntity : schemaEntities) {
if (!availableSchemaNames.contains(schemaEntity.name())) {
continue;
}
Map<String, String> entityProps = schemaEntity.properties();
if (entityProps != null
&& "true"
.equals(entityProps.get(SchemaOperationDispatcher.SCHEMA_CREATED_BY_GRAVITINO)))
{
return true;
}
try {
Schema schema =
catalogWrapper.doWithSchemaOps(ops ->
ops.loadSchema(schemaEntity.nameIdentifier()));
if (StringIdentifier.fromProperties(schema.properties()) != null) {
return true;
}
} catch (NoSuchSchemaException ex) {
LOG.warn(
"Schema {} no longer exists while checking whether it is
user-created",
schemaEntity.nameIdentifier(),
ex);
}
}
return false;
```
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -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));
+
Review Comment:
`testDropCatalog` now exercises multiple independent scenarios (baseline
drop, imported schema, missing schema, runtime error) in a single test method
with repeated catalog creation and re-stubbing. This makes failures harder to
diagnose and increases the chance of stub leakage between scenarios. Consider
splitting these into separate test methods (or factoring shared setup into
helpers) so each scenario is isolated and easier to reason about.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -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);
Review Comment:
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.
```suggestion
LOG.debug(
"Schema {} no longer exists while checking whether it is
user-created",
e.nameIdentifier());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
