This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new ff4b54f0a5 [Cherry-pick to branch-1.3] [#11401] fix(trino-connector):
drop_catalog falls back to server when local cache misses (#11454)
ff4b54f0a5 is described below
commit ff4b54f0a561386b6b45b993f89190fa7361a2c0
Author: Hongjun Liu <[email protected]>
AuthorDate: Fri Jun 5 20:01:20 2026 +0800
[Cherry-pick to branch-1.3] [#11401] fix(trino-connector): drop_catalog
falls back to server when local cache misses (#11454)
### What changes were proposed in this pull request?
Previously `DropCatalogStoredProcedure` only checked the Trino
connector's local cache (the `catalogConnectors` map) to decide whether
a catalog exists. When the local connector was null, it directly threw
"not exists"
without checking the Gravitino server.
This PR changes the null branch to call
`metalake.dropCatalog(catalogName, true)` on the server:
1. If the server returns true, the catalog is dropped on the server
successfully.
2. If the server returns false, the catalog does not exist on the server
either. We then respect the `ignore_not_exist` flag: return silently if
true, throw `GRAVITINO_CATALOG_NOT_EXISTS` if false.
The branch where the local connector is found stays the same as before.
Also adds a unit test class `TestDropCatalogStoredProcedure` for the new
code path and updates the Javadoc.
### Why are the changes needed?
Fix: #11401
The local cache may not have the catalog even though it exists on the
Gravitino server. This can happen when:
1. The catalog is created with invalid properties and the connector
fails to load.
2. The cache entry is evicted.
3. Trino is restarted before the background scheduler reloads it.
In this state the catalog cannot be dropped (local check returns null,
`drop_catalog` throws "not exists") and cannot be recreated either (the
server still holds the metadata, `create_catalog` throws "already
exists"). The
user has no way to recover without restarting Trino or calling the
server-side API directly.
By falling back to the server-side drop when the local cache misses, the
user can clean up the catalog with the same `drop_catalog` procedure.
### Does this PR introduce _any_ user-facing change?
No new API, configuration, or property key.
The behavior of `drop_catalog(catalog, ignore_not_exist)` changes only
for the bug case: calls that previously failed with "not exists" while
the catalog actually existed on the server now succeed. The other cases
(catalog
truly does not exist anywhere; catalog exists in the local cache) behave
the same as before.
### How was this patch tested?
Added a new unit test class `TestDropCatalogStoredProcedure` with three
cases:
1. The local cache does not contain the catalog but the server does. The
procedure should call the server-side drop and complete without error.
2. The catalog is absent from both the local cache and the server, with
`ignoreNotExist=false`. The procedure should throw
`GRAVITINO_CATALOG_NOT_EXISTS`.
3. Same as case 2 but with `ignoreNotExist=true`. The procedure should
return silently and must not trigger a local cache reload.
Ran `./gradlew :trino-connector:trino-connector:test -PskipITs` and
`./gradlew :trino-connector:trino-connector:spotlessApply`, both pass.
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): Support xxx"
- "[#233] fix: Check null before access result in xxx"
- "[MINOR] refactor: Fix typo in variable name"
- "[MINOR] docs: Fix typo in README"
- "[#255] test: Fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->
---
.../DropCatalogStoredProcedure.java | 27 +++--
.../TestDropCatalogStoredProcedure.java | 118 +++++++++++++++++++++
2 files changed, 137 insertions(+), 8 deletions(-)
diff --git
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java
index e57ac6e8fc..85feea0072 100644
---
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java
+++
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/system/storedprocedure/DropCatalogStoredProcedure.java
@@ -79,9 +79,11 @@ public class DropCatalogStoredProcedure extends
GravitinoStoredProcedure {
/**
* Drops the specified catalog.
*
+ * <p>If the catalog is not present in the local connector cache, falls back
to a server-side drop
+ * so that catalogs that failed to load can still be removed.
+ *
* @param catalogName the name of the catalog to drop
- * @param ignoreNotExist whether to ignore if the catalog does not exist
(only checked when the
- * catalog cannot be found initially)
+ * @param ignoreNotExist whether to ignore if the catalog does not exist
* @throws TrinoException if the catalog does not exist and ignoreNotExist
is false
*/
public void dropCatalog(String catalogName, boolean ignoreNotExist) {
@@ -90,14 +92,23 @@ public class DropCatalogStoredProcedure extends
GravitinoStoredProcedure {
catalogConnectorManager.getCatalogConnector(
catalogConnectorManager.getTrinoCatalogName(metalake,
catalogName));
if (catalogConnector == null) {
- if (ignoreNotExist) {
- return;
+ boolean dropped =
+
catalogConnectorManager.getMetalake(metalake).dropCatalog(catalogName, true);
+ if (!dropped) {
+ if (ignoreNotExist) {
+ return;
+ }
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_CATALOG_NOT_EXISTS,
+ "Catalog " + NameIdentifier.of(metalake, catalogName) + " not
exists.");
}
-
- throw new TrinoException(
- GravitinoErrorCode.GRAVITINO_CATALOG_NOT_EXISTS,
- "Catalog " + NameIdentifier.of(metalake, catalogName) + " not
exists.");
+ LOG.info(
+ "Drop catalog {} in metalake {} from server (no local connector)
successfully.",
+ catalogName,
+ metalake);
+ return;
}
+
// Always ignore "ignoreNotExist" inside Metalake.dropCatalog()
// because we already handled the null check above.
catalogConnector.getMetalake().dropCatalog(catalogName, true);
diff --git
a/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/system/storedprocedure/TestDropCatalogStoredProcedure.java
b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/system/storedprocedure/TestDropCatalogStoredProcedure.java
new file mode 100644
index 0000000000..c4b770c16e
--- /dev/null
+++
b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/system/storedprocedure/TestDropCatalogStoredProcedure.java
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.trino.connector.system.storedprocedure;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.trino.spi.TrinoException;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.trino.connector.GravitinoErrorCode;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorManager;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for {@link DropCatalogStoredProcedure} covering the server-side
fallback path added
+ * for issue #11401.
+ */
+public class TestDropCatalogStoredProcedure {
+
+ private static final String METALAKE = "test_metalake";
+ private static final String CATALOG = "test_catalog";
+ private static final String TRINO_CATALOG = "test_metalake.test_catalog";
+
+ @Test
+ public void
testDropCatalogFallsBackToServerWhenLocalCacheMissesAndServerHasIt() {
+ // The local connector cache does not have the catalog, but the Gravitino
server does.
+ // The procedure must call the server-side drop and succeed without
throwing "not exists".
+ CatalogConnectorManager manager = mock(CatalogConnectorManager.class);
+ GravitinoMetalake metalake = mock(GravitinoMetalake.class);
+
+ when(manager.getTrinoCatalogName(METALAKE,
CATALOG)).thenReturn(TRINO_CATALOG);
+ when(manager.getCatalogConnector(TRINO_CATALOG)).thenReturn(null);
+ when(manager.getMetalake(METALAKE)).thenReturn(metalake);
+ when(metalake.dropCatalog(CATALOG, true)).thenReturn(true);
+
+ DropCatalogStoredProcedure procedure = new
DropCatalogStoredProcedure(manager, METALAKE);
+
+ assertDoesNotThrow(() -> procedure.dropCatalog(CATALOG, false));
+
+ verify(metalake, times(1)).dropCatalog(CATALOG, true);
+ }
+
+ @Test
+ public void
testDropCatalogThrowsWhenAbsentEverywhereAndIgnoreNotExistFalse() {
+ // Catalog is absent from both the local cache and the server,
ignoreNotExist=false.
+ CatalogConnectorManager manager = mock(CatalogConnectorManager.class);
+ GravitinoMetalake metalake = mock(GravitinoMetalake.class);
+
+ when(manager.getTrinoCatalogName(METALAKE,
CATALOG)).thenReturn(TRINO_CATALOG);
+ when(manager.getCatalogConnector(TRINO_CATALOG)).thenReturn(null);
+ when(manager.getMetalake(METALAKE)).thenReturn(metalake);
+ when(metalake.dropCatalog(CATALOG, true)).thenReturn(false);
+
+ DropCatalogStoredProcedure procedure = new
DropCatalogStoredProcedure(manager, METALAKE);
+
+ // The internally-thrown "not exists" TrinoException is caught by the
outer catch(Exception)
+ // and re-wrapped as GRAVITINO_UNSUPPORTED_OPERATION, with the original
exception preserved
+ // as the cause. This matches the historical behavior observed in the
issue's stack trace.
+ // The contract we verify is: the user-visible message still tells them
the catalog does not
+ // exist, and the root cause carries the GRAVITINO_CATALOG_NOT_EXISTS
error code.
+ TrinoException error =
+ assertThrows(TrinoException.class, () ->
procedure.dropCatalog(CATALOG, false));
+ assertTrue(
+ error.getMessage().contains("not exists"),
+ () -> "Expected message to contain 'not exists' but was: " +
error.getMessage());
+ Throwable cause = error.getCause();
+ assertTrue(
+ cause instanceof TrinoException, () -> "Expected TrinoException cause,
was: " + cause);
+ assertEquals(
+ GravitinoErrorCode.GRAVITINO_CATALOG_NOT_EXISTS.toErrorCode(),
+ ((TrinoException) cause).getErrorCode());
+ }
+
+ @Test
+ public void
testDropCatalogReturnsSilentlyWhenAbsentEverywhereAndIgnoreNotExistTrue() {
+ // Catalog is absent from both the local cache and the server,
ignoreNotExist=true.
+ CatalogConnectorManager manager = mock(CatalogConnectorManager.class);
+ GravitinoMetalake metalake = mock(GravitinoMetalake.class);
+
+ when(manager.getTrinoCatalogName(METALAKE,
CATALOG)).thenReturn(TRINO_CATALOG);
+ when(manager.getCatalogConnector(TRINO_CATALOG)).thenReturn(null);
+ when(manager.getMetalake(METALAKE)).thenReturn(metalake);
+ when(metalake.dropCatalog(CATALOG, true)).thenReturn(false);
+
+ DropCatalogStoredProcedure procedure = new
DropCatalogStoredProcedure(manager, METALAKE);
+
+ assertDoesNotThrow(() -> procedure.dropCatalog(CATALOG, true));
+
+ // We DID consult the server, but no exception was thrown.
+ verify(metalake, times(1)).dropCatalog(CATALOG, true);
+ // Local-cache reload should NOT be triggered when the local connector was
already absent.
+ verify(manager, never()).catalogConnectorExist(anyString());
+ }
+}
