This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new ddf617b6d5 [#12232] fix(core): Preserve metadata during concurrent
drop and rename (#12235)
ddf617b6d5 is described below
commit ddf617b6d5199d7b3cda1cb68692a8d4f7e1dce1
Author: Qi Yu <[email protected]>
AuthorDate: Mon Aug 24 19:50:18 2026 +0800
[#12232] fix(core): Preserve metadata during concurrent drop and rename
(#12235)
### What changes were proposed in this pull request?
- Delete unmanaged table, view, and schema registrations only when the
external catalog confirms the drop or purge.
- Surface store failures around external table and view renames instead
of reporting success.
- Add regression tests for the related drop, purge, and rename paths.
### Why are the changes needed?
A concurrent drop and rename can cause the drop to return `false`
because the object has already been renamed externally. Deleting the old
stored registration in this case removes metadata associated with the
surviving object.
The same two-phase external-catalog and Gravitino-store behavior exists
for views, and an out-of-band schema rename has the corresponding
stale-name drop risk. Table purge also followed the unsafe deletion
path.
The rename paths could additionally perform an external rename when the
stored registration could not be read or updated, leaving inconsistent
state while reporting success.
Fix: #12232
### Does this PR introduce _any_ user-facing change?
Yes. For unmanaged tables, views, and schemas, a drop or purge that
returns `false` no longer deletes the stored registration. Table and
view renames now return an error when Gravitino cannot safely read or
update the stored registration.
No API or configuration changes are introduced.
### How was this patch tested?
- Added regression coverage to `TestTableOperationDispatcher`,
`TestViewOperationDispatcher`, and `TestSchemaOperationDispatcher`.
- Ran `./gradlew spotlessApply`.
- Ran `./gradlew :core:check -PskipITs -PskipDockerTests=true`.
- Ran `git diff --check`.
---
.../catalog/SchemaOperationDispatcher.java | 26 +++---
.../catalog/TableOperationDispatcher.java | 93 ++++++++++++++--------
.../gravitino/catalog/ViewOperationDispatcher.java | 63 +++++++++++----
.../catalog/TestSchemaOperationDispatcher.java | 18 +++++
.../catalog/TestTableOperationDispatcher.java | 87 ++++++++++++++++++--
.../catalog/TestViewOperationDispatcher.java | 71 ++++++++++++++++-
6 files changed, 288 insertions(+), 70 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
index 31c752b21b..695f244aec 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
@@ -344,20 +344,18 @@ public class SchemaOperationDispatcher extends
OperationDispatcher implements Sc
return droppedFromCatalog;
}
- // For the unmanaged schema, it could happen that the schema:
- // 1. It's not found in the catalog (dropped directly from
underlying sources)
- // 2. It's found in the catalog but not in the store (not managed by
Gravitino)
- // 3. It's found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store,
but we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, SCHEMA, true);
- } catch (NoSuchEntityException e) {
- LOG.warn("The schema to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result is ambiguous: the external schema may have been
renamed or dropped out
+ // of band. Preserve the registration because deleting it after a
rename would lose
+ // Gravitino-only metadata. A true out-of-band drop can therefore
leave a stale
+ // registration that requires separate cleanup.
+ if (droppedFromCatalog) {
+ try {
+ store.delete(ident, SCHEMA, true);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The schema to be dropped does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
SchemaEntityCleaner.deleteOrphanedSchemaEntities(
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
index e2d7bd9d86..a53722c4f4 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
@@ -33,6 +33,7 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
@@ -46,6 +47,7 @@ import org.apache.gravitino.Namespace;
import org.apache.gravitino.StringIdentifier;
import org.apache.gravitino.connector.HasPropertyMetadata;
import org.apache.gravitino.connector.capability.Capability;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.exceptions.NoSuchTableException;
@@ -243,11 +245,15 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
* @return The altered {@link Table} object after applying the changes.
* @throws NoSuchTableException If the table to alter does not exist.
* @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ * @throws GravitinoRuntimeException If a rename succeeds in the external
catalog but Gravitino
+ * cannot update the stored table registration consistently.
*/
@Override
public Table alterTable(NameIdentifier ident, TableChange... changes)
throws NoSuchTableException, IllegalArgumentException {
validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ boolean isRenameTable =
+ Arrays.stream(changes).anyMatch(change -> change instanceof
TableChange.RenameTable);
// use the read lock on the table if there does not exist
TableChange.RenameTable in the
// changes, or:
@@ -276,6 +282,11 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
() -> {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean isManagedTable = isManagedEntity(catalogIdent,
Capability.Scope.TABLE);
+ Optional<TableEntity> tableEntityBeforeRename =
+ isRenameTable && !isManagedTable
+ ? getTableEntityBeforeRename(ident)
+ : Optional.empty();
Table alteredTable =
doWithCatalog(
catalogIdent,
@@ -285,7 +296,6 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
NoSuchTableException.class,
IllegalArgumentException.class);
- boolean isManagedTable = isManagedEntity(catalogIdent,
Capability.Scope.TABLE);
if (isManagedTable) {
return EntityCombinedTable.of(alteredTable)
.withHiddenProperties(
@@ -297,9 +307,11 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
StringIdentifier stringId =
getStringIdFromProperties(alteredTable.properties());
// Case 1: The table is not created by Gravitino and this table is
never imported.
- TableEntity te = null;
+ TableEntity te = tableEntityBeforeRename.orElse(null);
if (stringId == null) {
- te = getEntity(ident, TABLE, TableEntity.class);
+ if (te == null) {
+ te = getEntity(ident, TABLE, TableEntity.class);
+ }
if (te == null) {
return EntityCombinedTable.of(alteredTable)
.withHiddenProperties(
@@ -350,6 +362,17 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
"UPDATE",
tableId);
+ // operateOnEntity returns null if the store update fails or if the
returned entity ID
+ // does not match the ID from the external catalog.
+ if (isRenameTable && updatedTableEntity == null) {
+ NameIdentifier newIdent =
+ NameIdentifier.of(getNewNamespace(ident, changes),
alteredTable.name());
+ throw new GravitinoRuntimeException(
+ "Table %s was renamed to %s in the external catalog, but its
registration in "
+ + "Gravitino could not be updated consistently",
+ ident, newIdent);
+ }
+
return EntityCombinedTable.of(alteredTable, updatedTableEntity)
.withHiddenProperties(
getHiddenPropertyNames(
@@ -386,20 +409,18 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
return droppedFromCatalog;
}
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the table from the store, but
we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result is ambiguous: the external table may have been
renamed or dropped out of
+ // band. Preserve the registration because deleting it after a
rename would lose
+ // Gravitino-only metadata. A true out-of-band drop can therefore
leave a stale
+ // registration that requires separate cleanup.
+ if (droppedFromCatalog) {
+ try {
+ store.delete(ident, TABLE);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
// Run unconditionally: an out-of-band drop may have left orphaned
schema entities. The
// cleanup is best-effort and stops as soon as a schema still exists.
@@ -442,21 +463,18 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
return droppedFromCatalog;
}
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the table from the store, but
we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be purged does not exist in the store: {}",
ident, e);
- return false;
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result is ambiguous: the external table may have been
renamed or dropped out of
+ // band. Preserve the registration because deleting it after a
rename would lose
+ // Gravitino-only metadata. A true out-of-band purge can therefore
leave a stale
+ // registration that requires separate cleanup.
+ if (droppedFromCatalog) {
+ try {
+ store.delete(ident, TABLE);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The table to be purged does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
// Run unconditionally: an out-of-band purge may have left orphaned
schema entities. The
// cleanup is best-effort and stops as soon as a schema still exists.
@@ -483,6 +501,17 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
.orElse(tableIdent.namespace());
}
+ private Optional<TableEntity> getTableEntityBeforeRename(NameIdentifier
ident) {
+ try {
+ return Optional.of(store.get(ident, TABLE, TableEntity.class));
+ } catch (NoSuchEntityException e) {
+ return Optional.empty();
+ } catch (Exception e) {
+ throw new GravitinoRuntimeException(
+ e, "Failed to read the stored registration for table %s before
renaming it", ident);
+ }
+ }
+
private EntityCombinedTable importTable(NameIdentifier identifier) {
EntityCombinedTable table = internalLoadTable(identifier);
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java
index 731639dca9..d2c17fabdc 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java
@@ -29,6 +29,7 @@ import java.time.Instant;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.function.Supplier;
import javax.annotation.Nullable;
import org.apache.gravitino.EntityAlreadyExistsException;
@@ -39,6 +40,7 @@ import org.apache.gravitino.Namespace;
import org.apache.gravitino.StringIdentifier;
import org.apache.gravitino.connector.HasPropertyMetadata;
import org.apache.gravitino.connector.capability.Capability;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.exceptions.NoSuchViewException;
@@ -209,11 +211,15 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
* @return The altered {@link View} object after applying the changes.
* @throws NoSuchViewException If the view to alter does not exist.
* @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ * @throws GravitinoRuntimeException If a rename succeeds in the external
catalog but Gravitino
+ * cannot update the stored view registration consistently.
*/
@Override
public View alterView(NameIdentifier ident, ViewChange... changes)
throws NoSuchViewException, IllegalArgumentException {
validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ boolean isRenameView =
+ Arrays.stream(changes).anyMatch(change -> change instanceof
ViewChange.RenameView);
NameIdentifier lockIdent = ident;
for (ViewChange change : changes) {
if (change instanceof ViewChange.RenameView) {
@@ -228,6 +234,9 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
() -> {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ Optional<ViewEntity> viewEntityBeforeRename =
+ isRenameView && !isManagedView ?
getViewEntityBeforeRename(ident) : Optional.empty();
View alteredView =
doWithCatalog(
catalogIdent,
@@ -237,7 +246,6 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
NoSuchViewException.class,
IllegalArgumentException.class);
- boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
if (isManagedView) {
return EntityCombinedView.of(alteredView)
.withHiddenProperties(
@@ -249,9 +257,11 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
StringIdentifier stringId =
getStringIdFromProperties(alteredView.properties());
// Case 1: The view is not created by Gravitino and this view is
never imported.
- ViewEntity existing = null;
+ ViewEntity existing = viewEntityBeforeRename.orElse(null);
if (stringId == null) {
- existing = getEntity(ident, VIEW, ViewEntity.class);
+ if (existing == null) {
+ existing = getEntity(ident, VIEW, ViewEntity.class);
+ }
if (existing == null) {
return EntityCombinedView.of(alteredView)
.withHiddenProperties(
@@ -275,6 +285,16 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
"UPDATE",
viewId);
+ // operateOnEntity returns null if the store update fails or if the
returned entity ID
+ // does not match the ID from the external catalog.
+ if (isRenameView && updatedViewEntity == null) {
+ NameIdentifier newIdent = NameIdentifier.of(ident.namespace(),
alteredView.name());
+ throw new GravitinoRuntimeException(
+ "View %s was renamed to %s in the external catalog, but its
registration in "
+ + "Gravitino could not be updated consistently",
+ ident, newIdent);
+ }
+
return EntityCombinedView.of(alteredView, updatedViewEntity)
.withHiddenProperties(
getHiddenPropertyNames(
@@ -311,20 +331,18 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
return droppedFromCatalog;
}
- // For unmanaged view, it could happen that the view:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the view from the store, but
we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, VIEW);
- } catch (NoSuchEntityException e) {
- LOG.warn("The view to be dropped does not exist in the store: {}",
ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result is ambiguous: the external view may have been
renamed or dropped out of
+ // band. Preserve the registration because deleting it after a
rename would lose
+ // Gravitino-only metadata. A true out-of-band drop can therefore
leave a stale
+ // registration that requires separate cleanup.
+ if (droppedFromCatalog) {
+ try {
+ store.delete(ident, VIEW);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The view to be dropped does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
// Run unconditionally: an out-of-band drop may have left orphaned
schema entities. The
// cleanup is best-effort and stops as soon as a schema still exists.
@@ -430,6 +448,17 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
catalogView.properties()));
}
+ private Optional<ViewEntity> getViewEntityBeforeRename(NameIdentifier ident)
{
+ try {
+ return Optional.of(store.get(ident, VIEW, ViewEntity.class));
+ } catch (NoSuchEntityException e) {
+ return Optional.empty();
+ } catch (Exception e) {
+ throw new GravitinoRuntimeException(
+ e, "Failed to read the stored registration for view %s before
renaming it", ident);
+ }
+ }
+
private EntityCombinedView internalLoadView(NameIdentifier ident) throws
NoSuchViewException {
NameIdentifier catalogIdentifier = getCatalogIdentifier(ident);
View view =
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java
b/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java
index bb2b0d0576..c5ebe8ff74 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java
@@ -46,7 +46,9 @@ import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.Schema;
import org.apache.gravitino.SchemaChange;
+import org.apache.gravitino.TestCatalog;
import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.connector.TestCatalogOperations;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.lock.LockManager;
import org.apache.gravitino.meta.AuditInfo;
@@ -370,6 +372,22 @@ public class TestSchemaOperationDispatcher extends
TestOperationDispatcher {
RuntimeException.class, () -> dispatcher.dropSchema(schemaIdent,
false));
}
+ @Test
+ public void testDropMissingSchemaPreservesStoredEntity() throws IOException {
+ reset(entityStore);
+ NameIdentifier schemaIdent = NameIdentifier.of(metalake, catalog,
"schema_renamed_out_of_band");
+ Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");
+ dispatcher.createSchema(schemaIdent, "comment", props);
+
+ TestCatalog testCatalog =
+ (TestCatalog) catalogManager.loadCatalog(NameIdentifier.of(metalake,
catalog));
+ TestCatalogOperations testCatalogOperations = (TestCatalogOperations)
testCatalog.ops();
+ Assertions.assertTrue(testCatalogOperations.dropSchema(schemaIdent,
false));
+
+ Assertions.assertFalse(dispatcher.dropSchema(schemaIdent, false));
+ Assertions.assertTrue(entityStore.exists(schemaIdent, SCHEMA));
+ }
+
@Test
public void testDropHierarchicalSchemaCleansUpOrphanedAncestors() throws
IOException {
// Clear any spy stubs leaked from other tests sharing the static
entityStore.
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
b/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
index a834398c71..0bf03cfb06 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
@@ -57,7 +57,9 @@ import org.apache.gravitino.TestCatalog;
import org.apache.gravitino.TestColumn;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.connector.TestCatalogOperations;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
+import org.apache.gravitino.exceptions.NoSuchTableException;
import org.apache.gravitino.lock.LockManager;
import org.apache.gravitino.meta.AuditInfo;
import org.apache.gravitino.meta.ColumnEntity;
@@ -505,6 +507,78 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertEquals("test", alteredTable4.auditInfo().lastModifier());
}
+ @Test
+ public void testRenameTableSurfacesStoreUpdateFailure() throws IOException {
+ Namespace tableNs = Namespace.of(metalake, catalog,
"schema_rename_store_failure");
+ NameIdentifier tableIdent = NameIdentifier.of(tableNs,
"table_before_rename");
+ NameIdentifier renamedTableIdent = NameIdentifier.of(tableNs,
"table_after_rename");
+ Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");
+ Column[] columns =
+ new Column[] {
+ TestColumn.builder()
+ .withName("col1")
+ .withPosition(0)
+ .withType(Types.StringType.get())
+ .build()
+ };
+
+
schemaOperationDispatcher.createSchema(NameIdentifier.of(tableNs.levels()),
"comment", props);
+ tableOperationDispatcher.createTable(tableIdent, columns, "comment",
props, new Transform[0]);
+
+ reset(entityStore);
+ doThrow(new NoSuchEntityException("mock update conflict"))
+ .when(entityStore)
+ .update(any(), any(), any(), any());
+
+ GravitinoRuntimeException exception =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () ->
+ tableOperationDispatcher.alterTable(
+ tableIdent, TableChange.rename(renamedTableIdent.name())));
+
Assertions.assertTrue(exception.getMessage().contains(tableIdent.toString()));
+
Assertions.assertTrue(exception.getMessage().contains(renamedTableIdent.toString()));
+ reset(entityStore);
+ }
+
+ @Test
+ public void testRenameTableFailsBeforeExternalChangeWhenStoreReadFails()
throws IOException {
+ Namespace tableNs = Namespace.of(metalake, catalog,
"schema_rename_store_read_failure");
+ NameIdentifier tableIdent = NameIdentifier.of(tableNs,
"table_before_failed_rename");
+ NameIdentifier renamedTableIdent = NameIdentifier.of(tableNs,
"table_after_failed_rename");
+ Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");
+ Column[] columns =
+ new Column[] {
+ TestColumn.builder()
+ .withName("col1")
+ .withPosition(0)
+ .withType(Types.StringType.get())
+ .build()
+ };
+
+
schemaOperationDispatcher.createSchema(NameIdentifier.of(tableNs.levels()),
"comment", props);
+ tableOperationDispatcher.createTable(tableIdent, columns, "comment",
props, new Transform[0]);
+
+ reset(entityStore);
+ doThrow(new IOException("mock store read failure"))
+ .when(entityStore)
+ .get(any(), eq(TABLE), any());
+
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () ->
+ tableOperationDispatcher.alterTable(
+ tableIdent, TableChange.rename(renamedTableIdent.name())));
+
+ TestCatalog testCatalog =
+ (TestCatalog) catalogManager.loadCatalog(NameIdentifier.of(metalake,
catalog));
+ TestCatalogOperations testCatalogOperations = (TestCatalogOperations)
testCatalog.ops();
+ Assertions.assertDoesNotThrow(() ->
testCatalogOperations.loadTable(tableIdent));
+ Assertions.assertThrows(
+ NoSuchTableException.class, () ->
testCatalogOperations.loadTable(renamedTableIdent));
+ reset(entityStore);
+ }
+
@Test
public void testCreateAndDropTable() throws IOException {
NameIdentifier tableIdent = NameIdentifier.of(metalake, catalog,
"schema71", "table31");
@@ -611,9 +685,11 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertFalse(testCatalogOperations.schemaExists(schemaIdent));
Assertions.assertFalse(testCatalogOperations.schemaExists(ancestorIdent));
- // dropTable returns false because the table is already gone from the
catalog, but the
- // orphaned schema entities must still be cleaned up.
+ // dropTable returns false because the table is already gone from the
catalog. Preserve the
+ // table entity because the same result can be caused by a concurrent
rename, while still
+ // cleaning up orphaned schema entities.
Assertions.assertFalse(tableOperationDispatcher.dropTable(tableIdent));
+ Assertions.assertTrue(entityStore.exists(tableIdent, TABLE));
Assertions.assertFalse(entityStore.exists(schemaIdent, SCHEMA));
Assertions.assertFalse(entityStore.exists(ancestorIdent, SCHEMA));
}
@@ -653,10 +729,11 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertFalse(testCatalogOperations.schemaExists(schemaIdent));
Assertions.assertFalse(testCatalogOperations.schemaExists(ancestorIdent));
- // purgeTable returns false because the table is already gone from the
catalog, but the
- // orphaned schema entities must still be cleaned up. A regression that
re-guards the cleanup
- // behind the catalog drop result would leave the stale schema entities
behind.
+ // purgeTable returns false because the table is already gone from the
catalog. Preserve the
+ // table entity because the same result can be caused by a concurrent
rename, while still
+ // cleaning up orphaned schema entities.
Assertions.assertFalse(tableOperationDispatcher.purgeTable(tableIdent));
+ Assertions.assertTrue(entityStore.exists(tableIdent, TABLE));
Assertions.assertFalse(entityStore.exists(schemaIdent, SCHEMA));
Assertions.assertFalse(entityStore.exists(ancestorIdent, SCHEMA));
}
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java
b/core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java
index 26372f662e..490e48052f 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java
@@ -25,8 +25,12 @@ import static org.apache.gravitino.Entity.EntityType.SCHEMA;
import static org.apache.gravitino.Entity.EntityType.VIEW;
import static org.apache.gravitino.StringIdentifier.ID_KEY;
import static org.apache.gravitino.TestBasePropertiesMetadata.COMMENT_KEY;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.reset;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
@@ -50,6 +54,7 @@ import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.TestCatalog;
import org.apache.gravitino.connector.TestCatalogOperations;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchViewException;
import org.apache.gravitino.lock.LockManager;
@@ -570,9 +575,11 @@ public class TestViewOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertFalse(testCatalogOperations.schemaExists(schemaIdent));
Assertions.assertFalse(testCatalogOperations.schemaExists(ancestorIdent));
- // dropView returns false because the view is already gone from the
catalog, but the
- // orphaned schema entities must still be cleaned up.
+ // dropView returns false because the view is already gone from the
catalog. Preserve the view
+ // entity because the same result can be caused by a concurrent rename,
while still cleaning up
+ // orphaned schema entities.
Assertions.assertFalse(viewOperationDispatcher.dropView(viewIdent));
+ Assertions.assertTrue(entityStore.exists(viewIdent, VIEW));
Assertions.assertFalse(entityStore.exists(schemaIdent, SCHEMA));
Assertions.assertFalse(entityStore.exists(ancestorIdent, SCHEMA));
}
@@ -649,6 +656,66 @@ public class TestViewOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertEquals("new_view", altered.name());
}
+ @Test
+ public void testRenameViewSurfacesStoreUpdateFailure() throws IOException {
+ Namespace viewNs = Namespace.of(metalake, catalog,
"schema_rename_view_store_failure");
+ schemaOperationDispatcher.createSchema(
+ NameIdentifier.of(viewNs.levels()), "c", ImmutableMap.of("k1", "v1",
"k2", "v2"));
+
+ NameIdentifier oldIdent = NameIdentifier.of(viewNs, "view_before_rename");
+ NameIdentifier newIdent = NameIdentifier.of(viewNs, "view_after_rename");
+ Representation[] representations = {
+ SQLRepresentation.builder().withDialect("spark").withSql("SELECT
1").build()
+ };
+ viewOperationDispatcher.createView(
+ oldIdent, "c", new Column[0], representations, null, null,
ImmutableMap.of("k1", "v1"));
+
+ reset(entityStore);
+ doThrow(new NoSuchEntityException("mock update conflict"))
+ .when(entityStore)
+ .update(any(), any(), any(), any());
+
+ GravitinoRuntimeException exception =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () -> viewOperationDispatcher.alterView(oldIdent,
ViewChange.rename(newIdent.name())));
+
Assertions.assertTrue(exception.getMessage().contains(oldIdent.toString()));
+
Assertions.assertTrue(exception.getMessage().contains(newIdent.toString()));
+ reset(entityStore);
+ }
+
+ @Test
+ public void testRenameViewFailsBeforeExternalChangeWhenStoreReadFails()
throws IOException {
+ Namespace viewNs = Namespace.of(metalake, catalog,
"schema_rename_view_store_read_failure");
+ schemaOperationDispatcher.createSchema(
+ NameIdentifier.of(viewNs.levels()), "c", ImmutableMap.of("k1", "v1",
"k2", "v2"));
+
+ NameIdentifier oldIdent = NameIdentifier.of(viewNs,
"view_before_failed_rename");
+ NameIdentifier newIdent = NameIdentifier.of(viewNs,
"view_after_failed_rename");
+ Representation[] representations = {
+ SQLRepresentation.builder().withDialect("spark").withSql("SELECT
1").build()
+ };
+ viewOperationDispatcher.createView(
+ oldIdent, "c", new Column[0], representations, null, null,
ImmutableMap.of("k1", "v1"));
+
+ reset(entityStore);
+ doThrow(new IOException("mock store read failure"))
+ .when(entityStore)
+ .get(any(), eq(VIEW), any());
+
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () -> viewOperationDispatcher.alterView(oldIdent,
ViewChange.rename(newIdent.name())));
+
+ TestCatalog testCatalog =
+ (TestCatalog) catalogManager.loadCatalog(NameIdentifier.of(metalake,
catalog));
+ TestCatalogOperations testCatalogOperations = (TestCatalogOperations)
testCatalog.ops();
+ Assertions.assertDoesNotThrow(() ->
testCatalogOperations.loadView(oldIdent));
+ Assertions.assertThrows(
+ NoSuchViewException.class, () ->
testCatalogOperations.loadView(newIdent));
+ reset(entityStore);
+ }
+
@Test
public void testAlterViewReplace() throws IOException {
Namespace viewNs = Namespace.of(metalake, catalog, "schema_alter_replace");
