[#3092] fix(relational-entity-store): Return false when deleting thro…

…w a `NoSuchEntityException` (#3311) ### What changes were proposed in this pull request? When Relational Entity Store deleteing an entity and throwing a NoSuchEntityException, return false finally. It's consistent with KV Store. ### Why are the changes needed? Fix: #3092 ### How was this patch tested? Add some duplicate deleted uts. Co-authored-by: xloya <[email protected]> Co-authored-by: xiaojiebao <[email protected]>
apache · May 9, 2024 · f29b1d2 · f29b1d2
1 parent 664ae0c
commit f29b1d2
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 1 deletion.
diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/RelationalEntityStore.java
@@ -102,7 +102,11 @@ public <E extends Entity & HasIdentifier> E get(
   @Override
   public boolean delete(NameIdentifier ident, Entity.EntityType entityType, boolean cascade)
       throws IOException {
-    return backend.delete(ident, entityType, cascade);
+    try {
+      return backend.delete(ident, entityType, cascade);
+    } catch (NoSuchEntityException nse) {
+      return false;
+    }
   }
 
   @Override

diff --git a/core/src/test/java/com/datastrato/gravitino/storage/TestEntityStorage.java b/core/src/test/java/com/datastrato/gravitino/storage/TestEntityStorage.java
@@ -1277,20 +1277,27 @@ private void validateDeleteTopicCascade(EntityStore store, TopicEntity topic1)
     // Delete the topic 'metalake.catalog.schema1.topic1'
     Assertions.assertTrue(store.delete(topic1.nameIdentifier(), Entity.EntityType.TOPIC));
     Assertions.assertFalse(store.exists(topic1.nameIdentifier(), Entity.EntityType.TOPIC));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(topic1.nameIdentifier(), Entity.EntityType.TOPIC));
   }
 
   private void validateDeleteFilesetCascade(EntityStore store, FilesetEntity fileset1)
       throws IOException {
     // Delete the fileset 'metalake.catalog.schema1.fileset1'
     Assertions.assertTrue(store.delete(fileset1.nameIdentifier(), Entity.EntityType.FILESET, true));
     Assertions.assertFalse(store.exists(fileset1.nameIdentifier(), Entity.EntityType.FILESET));
+    // Delete again should return false
+    Assertions.assertFalse(
+        store.delete(fileset1.nameIdentifier(), Entity.EntityType.FILESET, true));
   }
 
   private void validateDeleteTableCascade(EntityStore store, TableEntity table1)
       throws IOException {
     // Delete the table 'metalake.catalog.schema1.table1'
     Assertions.assertTrue(store.delete(table1.nameIdentifier(), Entity.EntityType.TABLE, true));
     Assertions.assertFalse(store.exists(table1.nameIdentifier(), Entity.EntityType.TABLE));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(table1.nameIdentifier(), Entity.EntityType.TABLE, true));
   }
 
   private void validateDeleteFileset(
@@ -1304,6 +1311,9 @@ private void validateDeleteFileset(
         store.delete(fileset1InSchema2.nameIdentifier(), Entity.EntityType.FILESET));
     Assertions.assertFalse(
         store.exists(fileset1InSchema2.nameIdentifier(), Entity.EntityType.FILESET));
+    // Delete again should return false
+    Assertions.assertFalse(
+        store.delete(fileset1InSchema2.nameIdentifier(), Entity.EntityType.FILESET));
 
     // Make sure fileset 'metalake.catalog.schema1.fileset1' still exist;
     Assertions.assertEquals(
@@ -1320,6 +1330,8 @@ private void validateDeleteTopic(
     // Delete the topic 'metalake.catalog.schema2.topic1'
     Assertions.assertTrue(store.delete(topic1InSchema2.nameIdentifier(), Entity.EntityType.TOPIC));
     Assertions.assertFalse(store.exists(topic1InSchema2.nameIdentifier(), Entity.EntityType.TOPIC));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(topic1InSchema2.nameIdentifier(), Entity.EntityType.TOPIC));
 
     // Make sure topic 'metalake.catalog.schema1.topic1' still exist;
     Assertions.assertEquals(
@@ -1352,6 +1364,10 @@ private void validateDeleteMetalakeCascade(
     Assertions.assertFalse(store.exists(userNew.nameIdentifier(), Entity.EntityType.USER));
     Assertions.assertFalse(store.exists(groupNew.nameIdentifier(), EntityType.GROUP));
     Assertions.assertFalse(store.exists(roleNew.nameIdentifier(), EntityType.ROLE));
+
+    // Delete again should return false
+    Assertions.assertFalse(
+        store.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE, true));
   }
 
   private void validateDeleteCatalogCascade(
@@ -1368,6 +1384,8 @@ private void validateDeleteCatalogCascade(
     Assertions.assertThrowsExactly(
         NoSuchEntityException.class,
         () -> store.get(schema2.nameIdentifier(), Entity.EntityType.SCHEMA, SchemaEntity.class));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(catalog.nameIdentifier(), Entity.EntityType.CATALOG, true));
   }
 
   private void validateDeleteSchemaCascade(
@@ -1419,6 +1437,8 @@ private void validateDeleteSchemaCascade(
       Assertions.assertTrue(e instanceof NoSuchEntityException);
       Assertions.assertTrue(e.getMessage().contains("schema1"));
     }
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(schema1.nameIdentifier(), Entity.EntityType.SCHEMA, true));
 
     Assertions.assertThrows(
         NoSuchEntityException.class,
@@ -1453,6 +1473,8 @@ private static void validateDeleteMetalake(
     Assertions.assertFalse(store.exists(user2.nameIdentifier(), Entity.EntityType.USER));
     Assertions.assertFalse(store.exists(group2.nameIdentifier(), Entity.EntityType.GROUP));
     Assertions.assertFalse(store.exists(role2.nameIdentifier(), Entity.EntityType.ROLE));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE));
   }
 
   private static void validateDeleteCatalog(
@@ -1488,6 +1510,8 @@ private static void validateDeleteCatalog(
 
     store.delete(catalog.nameIdentifier(), Entity.EntityType.CATALOG);
     Assertions.assertFalse(store.exists(catalog.nameIdentifier(), Entity.EntityType.CATALOG));
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(catalog.nameIdentifier(), Entity.EntityType.CATALOG));
   }
 
   private static void validateDeleteSchema(
@@ -1521,6 +1545,13 @@ private static void validateDeleteSchema(
     Assertions.assertFalse(store.exists(fileset1.nameIdentifier(), Entity.EntityType.FILESET));
     Assertions.assertFalse(store.exists(topic1.nameIdentifier(), Entity.EntityType.TOPIC));
     Assertions.assertFalse(store.exists(schema1.nameIdentifier(), Entity.EntityType.SCHEMA));
+
+    // Delete again should return false
+    Assertions.assertFalse(store.delete(table1.nameIdentifier(), Entity.EntityType.TABLE));
+    Assertions.assertFalse(store.delete(fileset1.nameIdentifier(), Entity.EntityType.FILESET));
+    Assertions.assertFalse(store.delete(topic1.nameIdentifier(), Entity.EntityType.TOPIC));
+    Assertions.assertFalse(store.delete(schema1.nameIdentifier(), Entity.EntityType.SCHEMA));
+
     // Now we re-insert schema1, table1, fileset1 and topic1, and everything should be OK
     SchemaEntity schema1New =
         createSchemaEntity(
@@ -1567,6 +1598,8 @@ private void validateDeleteUser(EntityStore store, UserEntity user1) throws IOEx
     Assertions.assertTrue(store.exists(user1.nameIdentifier(), Entity.EntityType.USER));
     Assertions.assertTrue(store.delete(user1.nameIdentifier(), Entity.EntityType.USER));
     Assertions.assertFalse(store.exists(user1.nameIdentifier(), Entity.EntityType.USER));
+    // delete again should return false
+    Assertions.assertFalse(store.delete(user1.nameIdentifier(), Entity.EntityType.USER));
 
     UserEntity user =
         createUser(RandomIdGenerator.INSTANCE.nextId(), "metalake", "user1", user1.auditInfo());
@@ -1577,6 +1610,8 @@ private void validateDeleteUser(EntityStore store, UserEntity user1) throws IOEx
   private void validateDeleteGroup(EntityStore store, GroupEntity group1) throws IOException {
     Assertions.assertTrue(store.delete(group1.nameIdentifier(), EntityType.GROUP));
     Assertions.assertFalse(store.exists(group1.nameIdentifier(), Entity.EntityType.GROUP));
+    // delete again should return false
+    Assertions.assertFalse(store.delete(group1.nameIdentifier(), Entity.EntityType.GROUP));
 
     GroupEntity group =
         createGroup(RandomIdGenerator.INSTANCE.nextId(), "metalake", "group1", group1.auditInfo());
@@ -1587,6 +1622,8 @@ private void validateDeleteGroup(EntityStore store, GroupEntity group1) throws I
   private void validateDeleteRole(EntityStore store, RoleEntity role1) throws IOException {
     Assertions.assertTrue(store.delete(role1.nameIdentifier(), EntityType.ROLE));
     Assertions.assertFalse(store.exists(role1.nameIdentifier(), Entity.EntityType.ROLE));
+    // delete again should return false
+    Assertions.assertFalse(store.delete(role1.nameIdentifier(), Entity.EntityType.ROLE));
 
     RoleEntity role =
         createRole(RandomIdGenerator.INSTANCE.nextId(), "metalake", "role1", role1.auditInfo());
@@ -1600,6 +1637,8 @@ private void validateDeleteTable(
     // Delete the table 'metalake.catalog.schema2.table1'
     Assertions.assertTrue(store.delete(table1InSchema2.nameIdentifier(), Entity.EntityType.TABLE));
     Assertions.assertFalse(store.exists(table1InSchema2.nameIdentifier(), Entity.EntityType.TABLE));
+    // delete again should return false
+    Assertions.assertFalse(store.delete(table1InSchema2.nameIdentifier(), Entity.EntityType.TABLE));
 
     // Make sure table 'metalake.catalog.schema1.table1' still exist;
     Assertions.assertEquals(