Skip to content

Commit

Permalink
[#5082] fix(catalog): fix clean bug after create catalog failed (#5084)
Browse files Browse the repository at this point in the history
### 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
  • Loading branch information
mchades authored and web-flow committed Oct 10, 2024
1 parent e9eaf31 commit e26c438
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 6 deletions.
1 change: 1 addition & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,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/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
Expand Down

0 comments on commit e26c438

Please sign in to comment.