This is an automated email from the ASF dual-hosted git repository. mchades pushed a commit to branch revert-10676-improvement/schema_entity_persistence_failure in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 1deacae759aa3d5ccd7a19aec9b5c679710d4f3c Author: mchades <[email protected]> AuthorDate: Mon Apr 13 19:53:00 2026 +0800 Revert "[#10656] improvement(core): Centralize schema persistence error messa…" This reverts commit 9c7fc94ffbb2526bb9036751e90b2b1d8de219c9. --- .../gravitino/catalog/OperationDispatcher.java | 50 ------------- .../catalog/SchemaOperationDispatcher.java | 28 ++----- .../catalog/TestSchemaOperationDispatcher.java | 86 ++++------------------ 3 files changed, 19 insertions(+), 145 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java index 5e10e101bb..006e05c64b 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java @@ -34,8 +34,6 @@ import org.apache.gravitino.StringIdentifier; import org.apache.gravitino.connector.HasPropertyMetadata; import org.apache.gravitino.connector.PropertiesMetadata; import org.apache.gravitino.connector.capability.Capability; -import org.apache.gravitino.exceptions.GravitinoRuntimeException; -import org.apache.gravitino.exceptions.NoSuchCatalogException; import org.apache.gravitino.exceptions.NoSuchEntityException; import org.apache.gravitino.file.FilesetChange; import org.apache.gravitino.messaging.TopicChange; @@ -131,42 +129,6 @@ public abstract class OperationDispatcher { } } - /** - * Attempts to rollback entity creation in the underlying catalog. - * - * <p>This method is used when entity metadata fails to persist to the Gravitino store after - * successful creation in the underlying catalog. It attempts to rollback the creation to maintain - * consistency between the catalog and Gravitino's metadata store. - * - * @param catalogIdent The identifier of the catalog. - * @param entityIdent The identifier of the entity to rollback. - * @param entityType The type of entity (e.g., "schema", "table", "topic") for logging purposes. - * @param rollbackOperation The operation to perform rollback. This should be a function that - * takes a CatalogWrapper and performs the necessary deletion operation (e.g., dropSchema, - * dropTable, etc.). - * @param <R> The return type of the rollback operation (usually Boolean for drop operations). - * @throws GravitinoRuntimeException If rollback fails. The exception will contain details about - * the rollback failure. - */ - protected <R> void rollbackEntityCreation( - NameIdentifier catalogIdent, - NameIdentifier entityIdent, - String entityType, - ThrowableFunction<CatalogManager.CatalogWrapper, R> rollbackOperation) { - try { - LOG.warn( - "Failed to persist {} metadata to Gravitino store for: {}. " - + "Attempting to rollback {} creation in underlying catalog to maintain consistency.", - entityType, - entityIdent, - entityType); - doWithCatalog(catalogIdent, rollbackOperation, NoSuchCatalogException.class); - LOG.info("Successfully rolled back {} creation for: {}", entityType, entityIdent); - } catch (Exception ex) { - throw ex; - } - } - protected Set<String> getHiddenPropertyNames( NameIdentifier catalogIdent, ThrowableFunction<HasPropertyMetadata, PropertiesMetadata> provider, @@ -327,17 +289,5 @@ public abstract class OperationDispatcher { + "identifier in the property {}, this is unexpected if this object is created by " + "Gravitino. This might be due to some operations that are not performed through Gravitino. " + "With this situation the returned object will not contain the metadata from Gravitino"; - - static final String ENTITY_PERSIST_FAILURE_WITH_ROLLBACK_FAILURE = - "Failed to persist schema metadata to Gravitino store for: %s. " - + "Additionally, rollback of schema creation in underlying catalog failed. " - + "The schema may exist in the underlying catalog but is not tracked by Gravitino."; - - static final String ENTITY_PERSIST_FAILURE_WITH_ROLLBACK_SUCCESS = - "Failed to persist schema metadata to Gravitino store for: %s. " - + "Schema creation in underlying catalog has been rolled back."; - - static final String ENTITY_SCHEMA_ROLLBACK_FAILED = - "Failed to rollback schema creation in underlying catalog for: {}."; } } 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 10d290f8b1..b61c99e0d7 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java @@ -19,7 +19,6 @@ package org.apache.gravitino.catalog; import static org.apache.gravitino.Entity.EntityType.SCHEMA; -import static org.apache.gravitino.catalog.OperationDispatcher.FormattedErrorMessages; import static org.apache.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForCreate; import static org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier; @@ -34,7 +33,6 @@ import org.apache.gravitino.SchemaChange; 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.NoSuchCatalogException; import org.apache.gravitino.exceptions.NoSuchEntityException; import org.apache.gravitino.exceptions.NoSuchSchemaException; @@ -159,26 +157,12 @@ public class SchemaOperationDispatcher extends OperationDispatcher implements Sc store.put(schemaEntity, true /* overwrite */); } catch (Exception e) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident, e); - - // Attempt to rollback the schema creation in the underlying catalog - try { - rollbackEntityCreation( - catalogIdent, - ident, - "schema", - catalogWrapper -> - catalogWrapper.doWithSchemaOps( - schemas -> schemas.dropSchema(ident, true /* cascade */))); - } catch (GravitinoRuntimeException ex) { - // Rollback failed - combine original exception with rollback failure - LOG.error(FormattedErrorMessages.ENTITY_SCHEMA_ROLLBACK_FAILED, ident, ex); - throw new GravitinoRuntimeException( - e, FormattedErrorMessages.ENTITY_PERSIST_FAILURE_WITH_ROLLBACK_FAILURE, ident); - } - - // Rollback succeeded - throw original exception with rollback success context - throw new GravitinoRuntimeException( - e, FormattedErrorMessages.ENTITY_PERSIST_FAILURE_WITH_ROLLBACK_SUCCESS, ident); + return EntityCombinedSchema.of(schema) + .withHiddenProperties( + getHiddenPropertyNames( + catalogIdent, + HasPropertyMetadata::schemaPropertiesMetadata, + schema.properties())); } // Merge both the metadata from catalog operation and the metadata from entity store. 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 bd32c4c735..f57a8c4038 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java @@ -28,8 +28,6 @@ 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 static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; import com.google.common.collect.ImmutableMap; import java.io.IOException; @@ -48,7 +46,6 @@ import org.apache.gravitino.Namespace; import org.apache.gravitino.Schema; import org.apache.gravitino.SchemaChange; import org.apache.gravitino.auth.AuthConstants; -import org.apache.gravitino.exceptions.GravitinoRuntimeException; import org.apache.gravitino.exceptions.NoSuchEntityException; import org.apache.gravitino.lock.LockManager; import org.apache.gravitino.meta.AuditInfo; @@ -117,82 +114,25 @@ public class TestSchemaOperationDispatcher extends TestOperationDispatcher { .filter(s -> s.name().equals("schema1")) .findFirst(); Assertions.assertTrue(ident1.isPresent()); - } - - @Test - public void testCreateSchemaFailsWhenStorePutFailsAndRollbackSucceeds() throws IOException { - Namespace ns = Namespace.of(metalake, catalog); - NameIdentifier schemaIdent = NameIdentifier.of(ns, "schema_rollback_success"); - Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2"); - - // Spy on the dispatcher to verify rollbackEntityCreation is called - SchemaOperationDispatcher spyDispatcher = spy(dispatcher); - - // Mock store.put() to fail - reset(entityStore); - doThrow(new IOException("Store failure due to network issue")) - .when(entityStore) - .put(any(), anyBoolean()); - // createSchema should fail with GravitinoRuntimeException after successful rollback - GravitinoRuntimeException exception = - Assertions.assertThrows( - GravitinoRuntimeException.class, - () -> spyDispatcher.createSchema(schemaIdent, "comment", props)); + // Test when the entity store failed to put the schema entity + doThrow(new IOException()).when(entityStore).put(any(), anyBoolean()); + NameIdentifier schemaIdent2 = NameIdentifier.of(ns, "schema2"); + Schema schema2 = dispatcher.createSchema(schemaIdent2, "comment", props); - // Verify the original cause is preserved - Assertions.assertEquals( - "Store failure due to network issue", exception.getCause().getMessage()); - - // Verify that rollbackEntityCreation was called - verify(spyDispatcher).rollbackEntityCreation(any(), eq(schemaIdent), eq("schema"), any()); + // Check if the created Schema's field values are correct + Assertions.assertEquals("schema2", schema2.name()); + Assertions.assertEquals("comment", schema2.comment()); + testProperties(props, schema2.properties()); - // Verify that the schema entity is NOT in the store - Assertions.assertFalse(entityStore.exists(schemaIdent, SCHEMA)); + // Check if the Schema entity is stored in the EntityStore + Assertions.assertFalse(entityStore.exists(schemaIdent2, SCHEMA)); Assertions.assertThrows( NoSuchEntityException.class, - () -> entityStore.get(schemaIdent, SCHEMA, SchemaEntity.class)); - } + () -> entityStore.get(schemaIdent2, SCHEMA, SchemaEntity.class)); - @Test - public void testCreateSchemaFailsWhenStorePutFailsAndRollbackFails() throws Exception { - Namespace ns = Namespace.of(metalake, catalog); - NameIdentifier schemaIdent = NameIdentifier.of(ns, "schema_rollback_fail"); - Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2"); - - // Spy on the dispatcher to make rollbackEntityCreation throw an exception - SchemaOperationDispatcher spyDispatcher = spy(dispatcher); - - // Make rollbackEntityCreation throw an exception to simulate rollback failure - doThrow(new GravitinoRuntimeException("Rollback failed - connection lost")) - .when(spyDispatcher) - .rollbackEntityCreation(any(), any(), any(), any()); - - // Mock store.put() to fail after successful schema creation - reset(entityStore); - IOException storeException = new IOException("Store failure due to network issue"); - doThrow(storeException).when(entityStore).put(any(), anyBoolean()); - - // createSchema should fail with GravitinoRuntimeException when rollback fails - GravitinoRuntimeException exception = - Assertions.assertThrows( - GravitinoRuntimeException.class, - () -> spyDispatcher.createSchema(schemaIdent, "comment", props)); - - // Verify the original cause is preserved (the IOException from store.put) - Assertions.assertEquals( - "Store failure due to network issue", exception.getCause().getMessage()); - - // Verify that rollbackEntityCreation was called - Assertions.assertThrows( - Exception.class, - () -> spyDispatcher.rollbackEntityCreation(any(), eq(schemaIdent), eq("schema"), any())); - - // Verify that the schema entity is NOT in the store - Assertions.assertFalse(entityStore.exists(schemaIdent, SCHEMA)); - Assertions.assertThrows( - NoSuchEntityException.class, - () -> entityStore.get(schemaIdent, SCHEMA, SchemaEntity.class)); + // Audit info is gotten from the catalog, not from the entity store + Assertions.assertEquals("test", schema2.auditInfo().creator()); } @Test
