Copilot commented on code in PR #11432:
URL: https://github.com/apache/gravitino/pull/11432#discussion_r3356108673
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java:
##########
@@ -90,13 +90,16 @@ public void dropCatalog(String catalogName, boolean
ignoreNotExist) {
catalogConnectorManager.getCatalogConnector(
catalogConnectorManager.getTrinoCatalogName(metalake,
catalogName));
if (catalogConnector == null) {
- if (ignoreNotExist) {
- return;
+ // Local cache miss — the catalog may still exist on the Gravitino
server (e.g., connector
+ // failed to load after creation). Fall back to a direct server-side
drop so zombie catalogs
+ // can be cleaned up.
+ if
(!catalogConnectorManager.getMetalake(metalake).dropCatalog(catalogName, true)
+ && !ignoreNotExist) {
Review Comment:
This new behavior (fall back to a server-side drop when the local connector
cache misses) is not covered by automated tests. Please add unit tests that
cover: (1) cache miss + server drop returns true => succeeds; (2) cache miss +
server drop returns false => throws when ignoreNotExist=false; (3) cache miss +
server drop returns false => no throw when ignoreNotExist=true.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java:
##########
@@ -90,13 +90,16 @@ public void dropCatalog(String catalogName, boolean
ignoreNotExist) {
catalogConnectorManager.getCatalogConnector(
catalogConnectorManager.getTrinoCatalogName(metalake,
catalogName));
if (catalogConnector == null) {
- if (ignoreNotExist) {
- return;
+ // Local cache miss — the catalog may still exist on the Gravitino
server (e.g., connector
+ // failed to load after creation). Fall back to a direct server-side
drop so zombie catalogs
+ // can be cleaned up.
+ if
(!catalogConnectorManager.getMetalake(metalake).dropCatalog(catalogName, true)
+ && !ignoreNotExist) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_CATALOG_NOT_EXISTS,
+ "Catalog " + NameIdentifier.of(metalake, catalogName) + " not
exists.");
Review Comment:
In the cache-miss path, the code drops the catalog on the server and returns
without syncing the connector manager state. If a background load of this
catalog races with this procedure (or completes right after the initial cache
lookup), the catalog connector could end up loaded locally even though the
server-side metadata has been dropped, until the next scheduled refresh.
Consider calling loadMetalakeSync() and verifying the connector is absent after
the server-side drop (similar to the non-null path) to make the drop behavior
deterministic.
--
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]
