[#5082] fix(catalog): fix clean bug after create catalog failed (#5084)

### What changes were proposed in this pull request? - not clean catalog if catalog already exists ### Why are the changes needed? we will clean the catalog when the creation fails, but if the catalog already exists it will also be dropped which is unexpected. Fix: #5082 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added
apache · Oct 10, 2024 · 12fc3f8 · 12fc3f8
1 parent aa481dc
commit 12fc3f8
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 6 deletions.
diff --git a/build.gradle.kts b/build.gradle.kts
@@ -513,6 +513,7 @@ tasks.rat {
     "ROADMAP.md",
     "clients/client-python/.pytest_cache/*",
     "clients/client-python/.venv/*",
+    "clients/client-python/venv/*",
     "clients/client-python/apache_gravitino.egg-info/*",
     "clients/client-python/gravitino/utils/http_client.py",
     "clients/client-python/tests/unittests/htmlcov/*",

diff --git a/...nts/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java b/...nts/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java
@@ -28,6 +28,7 @@
 import org.apache.gravitino.Catalog;
 import org.apache.gravitino.CatalogChange;
 import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.CatalogAlreadyExistsException;
 import org.apache.gravitino.integration.test.container.ContainerSuite;
 import org.apache.gravitino.integration.test.container.HiveContainer;
 import org.apache.gravitino.integration.test.util.AbstractIT;
@@ -125,6 +126,13 @@ public void testDropCatalog() {
     Assertions.assertTrue(metalake.catalogExists(catalogName));
     Assertions.assertEquals(catalogName, catalog.name());
 
+    Assertions.assertThrows(
+        CatalogAlreadyExistsException.class,
+        () ->
+            metalake.createCatalog(
+                catalogName, Catalog.Type.RELATIONAL, "hive", "catalog comment", properties));
+    Assertions.assertTrue(metalake.catalogExists(catalogName));
+
     Assertions.assertTrue(metalake.dropCatalog(catalogName), "catalog should be dropped");
     Assertions.assertFalse(metalake.dropCatalog(catalogName), "catalog should be non-existent");
   }

diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -366,7 +366,7 @@ public Catalog createCatalog(
                     .build())
             .build();
 
-    boolean createSuccess = false;
+    boolean needClean = true;
     try {
       NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels());
       if (!store.exists(metalakeIdent, EntityType.METALAKE)) {
@@ -376,22 +376,30 @@ public Catalog createCatalog(
 
       store.put(e, false /* overwrite */);
       CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e));
-      createSuccess = true;
+
+      needClean = false;
       return wrapper.catalog;
+
     } catch (EntityAlreadyExistsException e1) {
+      needClean = false;
       LOG.warn("Catalog {} already exists", ident, e1);
       throw new CatalogAlreadyExistsException("Catalog %s already exists", ident);
+
     } catch (IllegalArgumentException | NoSuchMetalakeException e2) {
       throw e2;
+
     } catch (Exception e3) {
       catalogCache.invalidate(ident);
       LOG.error("Failed to create catalog {}", ident, e3);
       if (e3 instanceof RuntimeException) {
         throw (RuntimeException) e3;
       }
       throw new RuntimeException(e3);
+
     } finally {
-      if (!createSuccess) {
+      if (needClean) {
+        // since we put the catalog entity into the store but failed to create the catalog instance,
+        // we need to clean up the entity stored.
         try {
           store.delete(ident, EntityType.CATALOG, true);
         } catch (IOException e4) {

diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -269,10 +269,10 @@ public void testCreateCatalog() {
 
     // test before creation
     Assertions.assertThrows(
-        NoSuchMetalakeException.class,
+        CatalogAlreadyExistsException.class,
         () ->
             catalogManager.testConnection(
-                ident2, Catalog.Type.RELATIONAL, provider, "comment", props));
+                ident, Catalog.Type.RELATIONAL, provider, "comment", props));
 
     // Test create with duplicated name
     Throwable exception2 =
@@ -309,7 +309,8 @@ public void testCreateCatalog() {
                 catalogManager.createCatalog(
                     failedIdent, Catalog.Type.RELATIONAL, provider, "comment", props));
     Assertions.assertTrue(
-        exception4.getMessage().contains("Properties are reserved and cannot be set"));
+        exception4.getMessage().contains("Properties are reserved and cannot be set"),
+        exception4.getMessage());
     Assertions.assertNull(catalogManager.catalogCache.getIfPresent(failedIdent));
   }