From 99b1cafeedbfc5137b0174d80249b19c33eda864 Mon Sep 17 00:00:00 2001 From: Heng Qin Date: Wed, 29 May 2024 18:49:54 +0800 Subject: [PATCH 01/15] [ISSUE-3607] Support to sync the entities --- .../gravitino/dto/rel/TableDTO.java | 2 +- .../datastrato/gravitino/GravitinoEnv.java | 17 +- .../catalog/EntityCombinedSchema.java | 15 ++ .../catalog/EntityCombinedTable.java | 15 ++ .../catalog/EntityCombinedTopic.java | 15 ++ .../gravitino/catalog/SchemaDispatcher.java | 5 +- .../catalog/SchemaNormalizeDispatcher.java | 6 + .../catalog/SchemaOperationDispatcher.java | 158 +++++++++++++----- .../gravitino/catalog/TableDispatcher.java | 5 +- .../catalog/TableNormalizeDispatcher.java | 6 + .../catalog/TableOperationDispatcher.java | 132 +++++++++++---- .../gravitino/catalog/TopicDispatcher.java | 5 +- .../catalog/TopicNormalizeDispatcher.java | 6 + .../catalog/TopicOperationDispatcher.java | 133 +++++++++++---- .../listener/SchemaEventDispatcher.java | 5 + .../listener/TableEventDispatcher.java | 5 + .../listener/TopicEventDispatcher.java | 5 + .../TestSchemaOperationDispatcher.java | 11 ++ .../catalog/TestTableOperationDispatcher.java | 13 ++ .../catalog/TestTopicOperationDispatcher.java | 13 ++ .../server/web/rest/RoleOperations.java | 83 +++++++++ .../server/web/rest/SchemaOperations.java | 4 +- .../server/web/rest/TableOperations.java | 4 +- .../server/web/rest/TopicOperations.java | 4 +- .../server/web/rest/TestRoleOperations.java | 4 + 25 files changed, 558 insertions(+), 113 deletions(-) diff --git a/common/src/main/java/com/datastrato/gravitino/dto/rel/TableDTO.java b/common/src/main/java/com/datastrato/gravitino/dto/rel/TableDTO.java index 84dd54b9917..2633ce34faf 100644 --- a/common/src/main/java/com/datastrato/gravitino/dto/rel/TableDTO.java +++ b/common/src/main/java/com/datastrato/gravitino/dto/rel/TableDTO.java @@ -58,7 +58,7 @@ private TableDTO() {} * @param properties The properties associated with the table. * @param audit The audit information for the table. * @param partitioning The partitioning of the table. - * @param indexes Teh indexes of the table. + * @param indexes The indexes of the table. */ private TableDTO( String name, diff --git a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java index a05bb3662ee..c48e0fcf02c 100644 --- a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java +++ b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java @@ -99,9 +99,9 @@ public static GravitinoEnv getInstance() { } /** - * This method is used for testing purposes only to set the lock manager for test in package - * `com.datastrato.gravitino.server.web.rest`, as tree lock depends on the lock manager and we did - * not mock the lock manager in the test, so we need to set the lock manager for test. + * This method is used for testing purposes only to set the lock manager for test in package as + * tree lock depends on the lock manager and we did not mock the lock manager in the test, so we + * need to set the lock manager for test. * * @param lockManager The lock manager to be set. */ @@ -121,6 +121,17 @@ public void setAccessControlManager(AccessControlManager accessControlManager) { this.accessControlManager = accessControlManager; } + /** + * This method is used for testing purposes only to set the access manager for test in package + * `com.datastrato.gravitino.server.web.rest`. + * + * @param catalogDispatcher The catalog dispatcher to be set. + */ + @VisibleForTesting + public void setCatalogDispatcher(CatalogDispatcher catalogDispatcher) { + this.catalogDispatcher = catalogDispatcher; + } + /** * This method is used for testing purposes only to set the entity store for test in package * `com.datastrato.gravitino.authorization`. diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java index 02e45e6eb81..f64ab40cc31 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java @@ -8,6 +8,7 @@ import com.datastrato.gravitino.Schema; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.SchemaEntity; +import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -23,6 +24,7 @@ public final class EntityCombinedSchema implements Schema { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + private boolean imported; private EntityCombinedSchema(Schema schema, SchemaEntity schemaEntity) { this.schema = schema; @@ -42,6 +44,11 @@ public EntityCombinedSchema withHiddenPropertiesSet(Set hiddenProperties return this; } + public EntityCombinedSchema withImported(boolean imported) { + this.imported = imported; + return this; + } + @Override public String name() { return schema.name(); @@ -73,4 +80,12 @@ public Audit auditInfo() { ? schema.auditInfo() : mergedAudit.merge(schemaEntity.auditInfo(), true /* overwrite */); } + + public boolean imported() { + return imported; + } + + Map schemaProperties() { + return Collections.unmodifiableMap(schema.properties()); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java index 593508f9e6e..d0e5efacfc8 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java @@ -14,6 +14,7 @@ import com.datastrato.gravitino.rel.expressions.sorts.SortOrder; import com.datastrato.gravitino.rel.expressions.transforms.Transform; import com.datastrato.gravitino.rel.indexes.Index; +import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -30,6 +31,7 @@ public final class EntityCombinedTable implements Table { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + private boolean imported; private EntityCombinedTable(Table table, TableEntity tableEntity) { this.table = table; @@ -49,6 +51,11 @@ public EntityCombinedTable withHiddenPropertiesSet(Set hiddenProperties) return this; } + public EntityCombinedTable withImported(boolean imported) { + this.imported = imported; + return this; + } + @Override public String name() { return table.name(); @@ -96,6 +103,10 @@ public Index[] index() { return table.index(); } + public boolean imported() { + return imported; + } + @Override public Audit auditInfo() { AuditInfo mergedAudit = @@ -110,4 +121,8 @@ public Audit auditInfo() { ? table.auditInfo() : mergedAudit.merge(tableEntity.auditInfo(), true /* overwrite */); } + + Map tableProperties() { + return Collections.unmodifiableMap(table.properties()); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java index b6fed19ec32..19bef7f63fc 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java @@ -8,6 +8,7 @@ import com.datastrato.gravitino.messaging.Topic; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.TopicEntity; +import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -23,6 +24,7 @@ public class EntityCombinedTopic implements Topic { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + private boolean imported; private EntityCombinedTopic(Topic topic, TopicEntity topicEntity) { this.topic = topic; @@ -42,6 +44,11 @@ public EntityCombinedTopic withHiddenPropertiesSet(Set hiddenProperties) return this; } + public EntityCombinedTopic withImported(boolean imported) { + this.imported = imported; + return this; + } + @Override public String name() { return topic.name(); @@ -73,4 +80,12 @@ public Audit auditInfo() { ? topic.auditInfo() : mergedAudit.merge(topicEntity.auditInfo(), true /* overwrite */); } + + public boolean imported() { + return imported; + } + + Map topicProperties() { + return Collections.unmodifiableMap(topic.properties()); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java index e61838e9556..3d92101017a 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java @@ -5,6 +5,7 @@ package com.datastrato.gravitino.catalog; +import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.connector.SupportsSchemas; /** @@ -13,4 +14,6 @@ * to dispatching or handling schema-related events or actions that are not covered by the standard * {@code SupportsSchemas} operations. */ -public interface SchemaDispatcher extends SupportsSchemas {} +public interface SchemaDispatcher extends SupportsSchemas { + boolean importSchema(NameIdentifier identifier); +} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java index 9c004989e80..32d1be56d76 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java @@ -71,6 +71,12 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty applyCaseSensitive(ident, Capability.Scope.SCHEMA, dispatcher), cascade); } + @Override + public boolean importSchema(NameIdentifier identifier) { + return dispatcher.importSchema( + applyCaseSensitive(identifier, Capability.Scope.SCHEMA, dispatcher)); + } + private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.SCHEMA, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index 1575ded111d..b5ab82276a2 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -20,6 +20,8 @@ import com.datastrato.gravitino.exceptions.NoSuchSchemaException; import com.datastrato.gravitino.exceptions.NonEmptySchemaException; import com.datastrato.gravitino.exceptions.SchemaAlreadyExistsException; +import com.datastrato.gravitino.lock.LockType; +import com.datastrato.gravitino.lock.TreeLockUtils; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.SchemaEntity; import com.datastrato.gravitino.storage.IdGenerator; @@ -159,47 +161,17 @@ public Schema createSchema(NameIdentifier ident, String comment, Map c.doWithSchemaOps(s -> s.loadSchema(ident)), - NoSuchSchemaException.class); + EntityCombinedSchema schema = + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedSchema(ident)); - // If the Schema is maintained by the Gravitino's store, we don't have to load again. - boolean isManagedSchema = isManagedEntity(catalogIdentifier, Capability.Scope.SCHEMA); - if (isManagedSchema) { - return EntityCombinedSchema.of(schema) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdentifier, - HasPropertyMetadata::schemaPropertiesMetadata, - schema.properties())); + if (schema.imported()) { + return schema; } - StringIdentifier stringId = getStringIdFromProperties(schema.properties()); - // Case 1: The schema is not created by Gravitino. - if (stringId == null) { - return EntityCombinedSchema.of(schema) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdentifier, - HasPropertyMetadata::schemaPropertiesMetadata, - schema.properties())); - } + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importSchema(ident)); - SchemaEntity schemaEntity = - operateOnEntity( - ident, - identifier -> store.get(identifier, SCHEMA, SchemaEntity.class), - "GET", - stringId.id()); - return EntityCombinedSchema.of(schema, schemaEntity) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdentifier, - HasPropertyMetadata::schemaPropertiesMetadata, - schema.properties())); + return schema; } /** @@ -242,7 +214,8 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())); + alteredSchema.properties())) + .withImported(true); } StringIdentifier stringId = getStringIdFromProperties(alteredSchema.properties()); @@ -253,7 +226,8 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())); + alteredSchema.properties())) + .withImported(isEntityExist(ident)); } SchemaEntity updatedSchemaEntity = @@ -280,12 +254,16 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) .build()), "UPDATE", stringId.id()); + + boolean imported = updatedSchemaEntity != null; + return EntityCombinedSchema.of(alteredSchema, updatedSchemaEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())); + alteredSchema.properties())) + .withImported(imported); } /** @@ -330,4 +308,104 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty ? droppedFromStore : droppedFromCatalog; } + + @Override + public boolean importSchema(NameIdentifier identifier) { + EntityCombinedSchema combinedSchema = loadCombinedSchema(identifier); + if (combinedSchema.imported()) { + return false; + } + + StringIdentifier stringId = getStringIdFromProperties(combinedSchema.schemaProperties()); + long uid; + if (stringId != null) { + // If the entity in the store doesn't match the external system, we use the data + // of external system to correct it. + uid = stringId.id(); + } else { + // If store doesn't exist entity, we sync the entity from the external system. + uid = idGenerator.nextId(); + } + + SchemaEntity schemaEntity = + SchemaEntity.builder() + .withId(uid) + .withName(identifier.name()) + .withNamespace(identifier.namespace()) + .withAuditInfo( + AuditInfo.builder() + .withCreator(combinedSchema.auditInfo().creator()) + .withCreateTime(combinedSchema.auditInfo().createTime()) + .withLastModifier(combinedSchema.auditInfo().lastModifier()) + .withLastModifiedTime(combinedSchema.auditInfo().lastModifiedTime()) + .build()) + .build(); + try { + store.put(schemaEntity, true); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); + throw new RuntimeException("Fail to access underlying storage"); + } + + return true; + } + + private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { + NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); + Schema schema = + doWithCatalog( + catalogIdentifier, + c -> c.doWithSchemaOps(s -> s.loadSchema(ident)), + NoSuchSchemaException.class); + + // If the Schema is maintained by the Gravitino's store, we don't have to load again. + boolean isManagedSchema = isManagedEntity(catalogIdentifier, Capability.Scope.SCHEMA); + if (isManagedSchema) { + return EntityCombinedSchema.of(schema) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdentifier, + HasPropertyMetadata::schemaPropertiesMetadata, + schema.properties())) + .withImported(true); + } + + StringIdentifier stringId = getStringIdFromProperties(schema.properties()); + // Case 1: The schema is not created by Gravitino. + if (stringId == null) { + return EntityCombinedSchema.of(schema) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdentifier, + HasPropertyMetadata::schemaPropertiesMetadata, + schema.properties())) + .withImported(isEntityExist(ident)); + } + + SchemaEntity schemaEntity = + operateOnEntity( + ident, + identifier -> store.get(identifier, SCHEMA, SchemaEntity.class), + "GET", + stringId.id()); + + boolean imported = schemaEntity != null; + + return EntityCombinedSchema.of(schema, schemaEntity) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdentifier, + HasPropertyMetadata::schemaPropertiesMetadata, + schema.properties())) + .withImported(imported); + } + + private boolean isEntityExist(NameIdentifier ident) { + try { + return store.exists(ident, SCHEMA); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); + throw new RuntimeException("Fail to access underlying storage"); + } + } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java index 7b54ccd5794..96a604d2a4f 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java @@ -5,6 +5,7 @@ package com.datastrato.gravitino.catalog; +import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.rel.TableCatalog; /** @@ -13,4 +14,6 @@ * dispatching or handling table-related events or actions that are not covered by the standard * {@code TableCatalog} operations. */ -public interface TableDispatcher extends TableCatalog {} +public interface TableDispatcher extends TableCatalog { + boolean importTable(NameIdentifier identifier); +} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java index ac7da6cbc55..aa1730fcfe0 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java @@ -99,6 +99,12 @@ public boolean tableExists(NameIdentifier ident) { return dispatcher.tableExists(applyCaseSensitive(ident, Capability.Scope.TABLE, dispatcher)); } + @Override + public boolean importTable(NameIdentifier identifier) { + return dispatcher.importTable( + applyCaseSensitive(identifier, Capability.Scope.TABLE, dispatcher)); + } + private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.TABLE, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index df41cdc2634..314781fafd6 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -19,6 +19,8 @@ import com.datastrato.gravitino.exceptions.NoSuchSchemaException; import com.datastrato.gravitino.exceptions.NoSuchTableException; import com.datastrato.gravitino.exceptions.TableAlreadyExistsException; +import com.datastrato.gravitino.lock.LockType; +import com.datastrato.gravitino.lock.TreeLockUtils; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.TableEntity; import com.datastrato.gravitino.rel.Column; @@ -79,37 +81,17 @@ public NameIdentifier[] listTables(Namespace namespace) throws NoSuchSchemaExcep */ @Override public Table loadTable(NameIdentifier ident) throws NoSuchTableException { - NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); - Table table = - doWithCatalog( - catalogIdentifier, - c -> c.doWithTableOps(t -> t.loadTable(ident)), - NoSuchTableException.class); + EntityCombinedTable table = + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTable(ident)); - StringIdentifier stringId = getStringIdFromProperties(table.properties()); - // Case 1: The table is not created by Gravitino. - if (stringId == null) { - return EntityCombinedTable.of(table) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdentifier, - HasPropertyMetadata::tablePropertiesMetadata, - table.properties())); + if (table.imported()) { + return table; } - TableEntity tableEntity = - operateOnEntity( - ident, - identifier -> store.get(identifier, TABLE, TableEntity.class), - "GET", - stringId.id()); + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTable(ident)); - return EntityCombinedTable.of(table, tableEntity) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdentifier, - HasPropertyMetadata::tablePropertiesMetadata, - table.properties())); + return table; } /** @@ -249,7 +231,8 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) getHiddenPropertyNames( getCatalogIdentifier(ident), HasPropertyMetadata::tablePropertiesMetadata, - alteredTable.properties())); + alteredTable.properties())) + .withImported(isEntityExist(ident)); } TableEntity updatedTableEntity = @@ -284,12 +267,15 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) "UPDATE", stringId.id()); + boolean imported = updatedTableEntity != null; + return EntityCombinedTable.of(alteredTable, updatedTableEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( getCatalogIdentifier(ident), HasPropertyMetadata::tablePropertiesMetadata, - alteredTable.properties())); + alteredTable.properties())) + .withImported(imported); } /** @@ -379,4 +365,92 @@ public boolean purgeTable(NameIdentifier ident) throws UnsupportedOperationExcep ? droppedFromStore : droppedFromCatalog; } + + @Override + public boolean importTable(NameIdentifier identifier) { + EntityCombinedTable combinedTable = loadCombinedTable(identifier); + + if (combinedTable.imported()) { + return false; + } + + StringIdentifier stringId = getStringIdFromProperties(combinedTable.tableProperties()); + long uid; + if (stringId != null) { + // If the entity in the store doesn't match the external system, we use the data + // of external system to correct it. + uid = stringId.id(); + } else { + // If store doesn't exist entity, we sync the entity from the external system. + uid = idGenerator.nextId(); + } + + TableEntity tableEntity = + TableEntity.builder() + .withId(uid) + .withName(identifier.name()) + .withNamespace(identifier.namespace()) + .withAuditInfo( + AuditInfo.builder() + .withCreator(combinedTable.auditInfo().creator()) + .withCreateTime(combinedTable.auditInfo().createTime()) + .withLastModifier(combinedTable.auditInfo().lastModifier()) + .withLastModifiedTime(combinedTable.auditInfo().lastModifiedTime()) + .build()) + .build(); + try { + store.put(tableEntity, true); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); + throw new RuntimeException("Fail to access underlying storage"); + } + return true; + } + + private boolean isEntityExist(NameIdentifier ident) { + try { + return store.exists(ident, TABLE); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); + throw new RuntimeException("Fail to access underlying storage"); + } + } + + private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { + NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); + Table table = + doWithCatalog( + catalogIdentifier, + c -> c.doWithTableOps(t -> t.loadTable(ident)), + NoSuchTableException.class); + + StringIdentifier stringId = getStringIdFromProperties(table.properties()); + // Case 1: The table is not created by Gravitino. + if (stringId == null) { + return EntityCombinedTable.of(table) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdentifier, + HasPropertyMetadata::tablePropertiesMetadata, + table.properties())) + .withImported(isEntityExist(ident)); + } + + TableEntity tableEntity = + operateOnEntity( + ident, + identifier -> store.get(identifier, TABLE, TableEntity.class), + "GET", + stringId.id()); + + boolean imported = tableEntity != null; + + return EntityCombinedTable.of(table, tableEntity) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdentifier, + HasPropertyMetadata::tablePropertiesMetadata, + table.properties())) + .withImported(imported); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java index 131a600c621..7d96c5abf95 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java @@ -5,6 +5,7 @@ package com.datastrato.gravitino.catalog; +import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.messaging.TopicCatalog; /** @@ -13,4 +14,6 @@ * dispatching or handling topic-related events or actions that are not covered by the standard * {@code TopicCatalog} operations. */ -public interface TopicDispatcher extends TopicCatalog {} +public interface TopicDispatcher extends TopicCatalog { + boolean importTopic(NameIdentifier identifier); +} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java index aa372b623c1..de05fc04cc4 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java @@ -72,6 +72,12 @@ public boolean dropTopic(NameIdentifier ident) { return dispatcher.dropTopic(applyCaseSensitive(ident, Capability.Scope.TOPIC, dispatcher)); } + @Override + public boolean importTopic(NameIdentifier identifier) { + return dispatcher.importTopic( + applyCaseSensitive(identifier, Capability.Scope.TOPIC, dispatcher)); + } + private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.TOPIC, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index 6d52e92d75a..998421d3cbf 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -18,6 +18,8 @@ import com.datastrato.gravitino.exceptions.NoSuchSchemaException; import com.datastrato.gravitino.exceptions.NoSuchTopicException; import com.datastrato.gravitino.exceptions.TopicAlreadyExistsException; +import com.datastrato.gravitino.lock.LockType; +import com.datastrato.gravitino.lock.TreeLockUtils; import com.datastrato.gravitino.messaging.DataLayout; import com.datastrato.gravitino.messaging.Topic; import com.datastrato.gravitino.messaging.TopicChange; @@ -70,36 +72,16 @@ public NameIdentifier[] listTopics(Namespace namespace) throws NoSuchSchemaExcep */ @Override public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { - NameIdentifier catalogIdent = getCatalogIdentifier(ident); - Topic topic = - doWithCatalog( - catalogIdent, - c -> c.doWithTopicOps(t -> t.loadTopic(ident)), - NoSuchTopicException.class); + EntityCombinedTopic topic = + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTopic(ident)); - StringIdentifier stringId = getStringIdFromProperties(topic.properties()); - // Case 1: The topic is not created by Gravitino. - // Note: for Kafka catalog, stringId will not be null. Because there is no way to store the - // Gravitino - // ID in Kafka, therefor we use the topic ID as the Gravitino ID - if (stringId == null) { - return EntityCombinedTopic.of(topic) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())); + if (topic.imported()) { + return topic; } - TopicEntity topicEntity = - operateOnEntity( - ident, - identifier -> store.get(identifier, TOPIC, TopicEntity.class), - "GET", - getStringIdFromProperties(topic.properties()).id()); - - return EntityCombinedTopic.of(topic, topicEntity) - .withHiddenPropertiesSet( - getHiddenPropertyNames( - catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())); + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTopic(ident)); + return topic; } /** @@ -237,12 +219,15 @@ public Topic alterTopic(NameIdentifier ident, TopicChange... changes) "UPDATE", getStringIdFromProperties(alteredTopic.properties()).id()); + boolean imported = updatedTopicEntity != null; + return EntityCombinedTopic.of(alteredTopic, updatedTopicEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, - alteredTopic.properties())); + alteredTopic.properties())) + .withImported(imported); } /** @@ -282,4 +267,96 @@ public boolean dropTopic(NameIdentifier ident) { ? droppedFromStore : droppedFromCatalog; } + + @Override + public boolean importTopic(NameIdentifier identifier) { + + EntityCombinedTopic combinedTopic = loadCombinedTopic(identifier); + + if (combinedTopic.imported()) { + return false; + } + + StringIdentifier stringId = getStringIdFromProperties(combinedTopic.topicProperties()); + + long uid; + if (stringId != null) { + // If the entity in the store doesn't match the external system, we use the data + // of external system to correct it. + uid = stringId.id(); + } else { + // If store doesn't exist entity, we sync the entity from the external system. + uid = idGenerator.nextId(); + } + + TopicEntity topicEntity = + TopicEntity.builder() + .withId(uid) + .withName(combinedTopic.name()) + .withComment(combinedTopic.comment()) + .withNamespace(identifier.namespace()) + .withAuditInfo( + AuditInfo.builder() + .withCreator(combinedTopic.auditInfo().creator()) + .withCreateTime(combinedTopic.auditInfo().createTime()) + .withLastModifier(combinedTopic.auditInfo().lastModifier()) + .withLastModifiedTime(combinedTopic.auditInfo().lastModifiedTime()) + .build()) + .build(); + + try { + store.put(topicEntity, true); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); + throw new RuntimeException("Fail to access underlying storage"); + } + + return true; + } + + private boolean isEntityExist(NameIdentifier ident) { + try { + return store.exists(ident, TOPIC); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); + throw new RuntimeException("Fail to access underlying storage"); + } + } + + private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { + NameIdentifier catalogIdent = getCatalogIdentifier(ident); + Topic topic = + doWithCatalog( + catalogIdent, + c -> c.doWithTopicOps(t -> t.loadTopic(ident)), + NoSuchTopicException.class); + + StringIdentifier stringId = getStringIdFromProperties(topic.properties()); + // Case 1: The topic is not created by Gravitino. + // Note: for Kafka catalog, stringId will not be null. Because there is no way to store the + // Gravitino + // ID in Kafka, therefor we use the topic ID as the Gravitino ID + if (stringId == null) { + return EntityCombinedTopic.of(topic) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())) + .withImported(isEntityExist(ident)); + } + + TopicEntity topicEntity = + operateOnEntity( + ident, + identifier -> store.get(identifier, TOPIC, TopicEntity.class), + "GET", + getStringIdFromProperties(topic.properties()).id()); + + boolean imported = topicEntity != null; + + return EntityCombinedTopic.of(topic, topicEntity) + .withHiddenPropertiesSet( + getHiddenPropertyNames( + catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())) + .withImported(imported); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java index 50c6df4ea18..21ec9a704db 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java @@ -130,4 +130,9 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty throw e; } } + + @Override + public boolean importSchema(NameIdentifier identifier) { + return dispatcher.importSchema(identifier); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java index deacbba2094..e1a78c35840 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java @@ -169,4 +169,9 @@ public boolean purgeTable(NameIdentifier ident) { public boolean tableExists(NameIdentifier ident) { return dispatcher.tableExists(ident); } + + @Override + public boolean importTable(NameIdentifier identifier) { + return dispatcher.importTable(identifier); + } } diff --git a/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java index dd628a534d2..61777703064 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java @@ -128,4 +128,9 @@ public Topic createTopic( throw e; } } + + @Override + public boolean importTopic(NameIdentifier identifier) { + return dispatcher.importTopic(identifier); + } } diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java index c04b8e1a72b..b8e8e303fa7 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java @@ -11,14 +11,19 @@ import static org.mockito.ArgumentMatchers.anyBoolean; 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.datastrato.gravitino.Config; +import com.datastrato.gravitino.Configs; +import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.Schema; import com.datastrato.gravitino.SchemaChange; import com.datastrato.gravitino.auth.AuthConstants; import com.datastrato.gravitino.exceptions.NoSuchEntityException; +import com.datastrato.gravitino.lock.LockManager; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.SchemaEntity; import com.google.common.collect.ImmutableMap; @@ -39,6 +44,12 @@ public class TestSchemaOperationDispatcher extends TestOperationDispatcher { @BeforeAll public static void initialize() throws IOException { dispatcher = new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); + + Config config = mock(Config.class); + doReturn(100000L).when(config).get(Configs.TREE_LOCK_MAX_NODE_IN_MEMORY); + doReturn(1000L).when(config).get(Configs.TREE_LOCK_MIN_NODE_IN_MEMORY); + doReturn(36000L).when(config).get(Configs.TREE_LOCK_CLEAN_INTERVAL); + GravitinoEnv.getInstance().setLockManager(new LockManager(config)); } @Test diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java index 83cee605232..f3c1324b671 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java @@ -4,6 +4,9 @@ */ package com.datastrato.gravitino.catalog; +import static com.datastrato.gravitino.Configs.TREE_LOCK_CLEAN_INTERVAL; +import static com.datastrato.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY; +import static com.datastrato.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY; import static com.datastrato.gravitino.Entity.EntityType.TABLE; import static com.datastrato.gravitino.StringIdentifier.ID_KEY; import static com.datastrato.gravitino.TestBasePropertiesMetadata.COMMENT_KEY; @@ -11,13 +14,17 @@ import static org.mockito.ArgumentMatchers.anyBoolean; 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.datastrato.gravitino.Config; +import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.TestColumn; import com.datastrato.gravitino.auth.AuthConstants; import com.datastrato.gravitino.exceptions.NoSuchEntityException; +import com.datastrato.gravitino.lock.LockManager; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.TableEntity; import com.datastrato.gravitino.rel.Column; @@ -46,6 +53,12 @@ public static void initialize() throws IOException { new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); tableOperationDispatcher = new TableOperationDispatcher(catalogManager, entityStore, idGenerator); + + Config config = mock(Config.class); + doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); + doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); + doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); + GravitinoEnv.getInstance().setLockManager(new LockManager(config)); } @Test diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java index fd06db7614a..ccf55f1a19d 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java @@ -4,18 +4,25 @@ */ package com.datastrato.gravitino.catalog; +import static com.datastrato.gravitino.Configs.TREE_LOCK_CLEAN_INTERVAL; +import static com.datastrato.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY; +import static com.datastrato.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY; import static com.datastrato.gravitino.StringIdentifier.ID_KEY; import static com.datastrato.gravitino.TestBasePropertiesMetadata.COMMENT_KEY; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; 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.datastrato.gravitino.Config; +import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.auth.AuthConstants; import com.datastrato.gravitino.exceptions.NoSuchEntityException; +import com.datastrato.gravitino.lock.LockManager; import com.datastrato.gravitino.messaging.Topic; import com.datastrato.gravitino.messaging.TopicChange; import com.datastrato.gravitino.meta.AuditInfo; @@ -39,6 +46,12 @@ public static void initialize() throws IOException { new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); topicOperationDispatcher = new TopicOperationDispatcher(catalogManager, entityStore, idGenerator); + + Config config = mock(Config.class); + doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); + doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); + doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); + GravitinoEnv.getInstance().setLockManager(new LockManager(config)); } @Test diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java index 147fa42abaa..bfc84ee0936 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java @@ -7,15 +7,23 @@ import com.codahale.metrics.annotation.ResponseMetered; import com.codahale.metrics.annotation.Timed; import com.datastrato.gravitino.GravitinoEnv; +import com.datastrato.gravitino.MetadataObject; +import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.authorization.AccessControlManager; import com.datastrato.gravitino.authorization.Privilege; import com.datastrato.gravitino.authorization.Privileges; import com.datastrato.gravitino.authorization.SecurableObject; import com.datastrato.gravitino.authorization.SecurableObjects; +import com.datastrato.gravitino.catalog.SchemaDispatcher; +import com.datastrato.gravitino.catalog.TableDispatcher; +import com.datastrato.gravitino.catalog.TopicDispatcher; +import com.datastrato.gravitino.dto.authorization.SecurableObjectDTO; import com.datastrato.gravitino.dto.requests.RoleCreateRequest; import com.datastrato.gravitino.dto.responses.DeleteResponse; import com.datastrato.gravitino.dto.responses.RoleResponse; import com.datastrato.gravitino.dto.util.DTOConverters; +import com.datastrato.gravitino.lock.LockType; +import com.datastrato.gravitino.lock.TreeLockUtils; import com.datastrato.gravitino.metrics.MetricNames; import com.datastrato.gravitino.server.web.Utils; import com.google.common.base.Preconditions; @@ -77,6 +85,10 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq request.getSecurableObjects() != null && request.getSecurableObjects().length == 1, "The size of securable objects must be 1"); + for (SecurableObjectDTO object : request.getSecurableObjects()) { + checkSecurableObjects(metalake, object); + } + return Utils.doAs( httpRequest, () -> { @@ -132,4 +144,75 @@ public Response deleteRole( return ExceptionHandlers.handleRoleException(OperationType.DELETE, role, metalake, e); } } + + // Check every securable object whether exists and is imported. + private void checkSecurableObjects(String metalake, SecurableObjectDTO object) { + NameIdentifier identifier; + + // Securable object ignores the metalake namespace, so we should add it back. + if (object.type() == MetadataObject.Type.METALAKE) { + identifier = NameIdentifier.parse(object.fullName()); + } else { + identifier = NameIdentifier.parse(String.format("%s.%s", metalake, object.fullName())); + } + + String existErrMsg = "Securable object % doesn't exist"; + + TreeLockUtils.doWithTreeLock( + identifier, + LockType.READ, + () -> { + switch (object.type()) { + case METALAKE: + if (!GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + + break; + case CATALOG: + if (!GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + + break; + case SCHEMA: + { + SchemaDispatcher dispatcher = GravitinoEnv.getInstance().schemaDispatcher(); + if (!dispatcher.schemaExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + break; + } + case FILESET: + if (!GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + + break; + case TABLE: + { + TableDispatcher dispatcher = GravitinoEnv.getInstance().tableDispatcher(); + if (!dispatcher.tableExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + + break; + } + case TOPIC: + { + TopicDispatcher dispatcher = GravitinoEnv.getInstance().topicDispatcher(); + if (!dispatcher.topicExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); + } + + break; + } + default: + throw new IllegalArgumentException( + String.format("Doesn't support the type %s", object.type())); + } + + return null; + }); + } } diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/SchemaOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/SchemaOperations.java index 4bdcb1c09c3..33531102b7c 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/SchemaOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/SchemaOperations.java @@ -133,9 +133,7 @@ public Response loadSchema( httpRequest, () -> { NameIdentifier ident = NameIdentifierUtil.ofSchema(metalake, catalog, schema); - Schema s = - TreeLockUtils.doWithTreeLock( - ident, LockType.READ, () -> dispatcher.loadSchema(ident)); + Schema s = dispatcher.loadSchema(ident); Response response = Utils.ok(new SchemaResponse(DTOConverters.toDTO(s))); LOG.info("Schema loaded: {}.{}.{}", metalake, catalog, s.name()); return response; diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/TableOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/TableOperations.java index b0093fd2b9d..42630da8c36 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/TableOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/TableOperations.java @@ -148,9 +148,7 @@ public Response loadTable( httpRequest, () -> { NameIdentifier ident = NameIdentifierUtil.ofTable(metalake, catalog, schema, table); - Table t = - TreeLockUtils.doWithTreeLock( - ident, LockType.READ, () -> dispatcher.loadTable(ident)); + Table t = dispatcher.loadTable(ident); Response response = Utils.ok(new TableResponse(DTOConverters.toDTO(t))); LOG.info("Table loaded: {}.{}.{}.{}", metalake, catalog, schema, table); return response; diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/TopicOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/TopicOperations.java index c49552ac8b5..1932585ef36 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/TopicOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/TopicOperations.java @@ -142,9 +142,7 @@ public Response loadTopic( () -> { LOG.info("Loading topic: {}.{}.{}.{}", metalake, catalog, schema, topic); NameIdentifier ident = NameIdentifierUtil.ofTopic(metalake, catalog, schema, topic); - Topic t = - TreeLockUtils.doWithTreeLock( - ident, LockType.READ, () -> dispatcher.loadTopic(ident)); + Topic t = dispatcher.loadTopic(ident); Response response = Utils.ok(new TopicResponse(DTOConverters.toDTO(t))); LOG.info("Topic loaded: {}.{}.{}.{}", metalake, catalog, schema, topic); return response; diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java index c32e201e85f..e8cdced23e6 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java @@ -19,6 +19,7 @@ import com.datastrato.gravitino.authorization.Role; import com.datastrato.gravitino.authorization.SecurableObject; import com.datastrato.gravitino.authorization.SecurableObjects; +import com.datastrato.gravitino.catalog.CatalogDispatcher; import com.datastrato.gravitino.dto.authorization.RoleDTO; import com.datastrato.gravitino.dto.authorization.SecurableObjectDTO; import com.datastrato.gravitino.dto.requests.RoleCreateRequest; @@ -55,6 +56,7 @@ public class TestRoleOperations extends JerseyTest { private static final AccessControlManager manager = mock(AccessControlManager.class); + private static final CatalogDispatcher dispatcher = mock(CatalogDispatcher.class); private static class MockServletRequestFactory extends ServletRequestFactoryBase { @Override @@ -73,6 +75,7 @@ public static void setup() { Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); GravitinoEnv.getInstance().setLockManager(new LockManager(config)); GravitinoEnv.getInstance().setAccessControlManager(manager); + GravitinoEnv.getInstance().setCatalogDispatcher(dispatcher); } @Override @@ -110,6 +113,7 @@ public void testCreateRole() { Role role = buildRole("role1"); when(manager.createRole(any(), any(), any(), any())).thenReturn(role); + when(dispatcher.catalogExists(any())).thenReturn(true); Response resp = target("/metalakes/metalake1/roles") From 3dac56c6ee40e89af67884fa8e7566601d8fe7d0 Mon Sep 17 00:00:00 2001 From: Heng Qin Date: Wed, 5 Jun 2024 11:28:30 +0800 Subject: [PATCH 02/15] revert interface --- .../com/datastrato/gravitino/catalog/SchemaDispatcher.java | 5 +---- .../gravitino/catalog/SchemaNormalizeDispatcher.java | 6 ------ .../gravitino/catalog/SchemaOperationDispatcher.java | 3 +-- .../com/datastrato/gravitino/catalog/TableDispatcher.java | 5 +---- .../gravitino/catalog/TableNormalizeDispatcher.java | 6 ------ .../gravitino/catalog/TableOperationDispatcher.java | 3 +-- .../com/datastrato/gravitino/catalog/TopicDispatcher.java | 5 +---- .../gravitino/catalog/TopicNormalizeDispatcher.java | 6 ------ .../gravitino/catalog/TopicOperationDispatcher.java | 3 +-- .../gravitino/listener/SchemaEventDispatcher.java | 5 ----- .../datastrato/gravitino/listener/TableEventDispatcher.java | 5 ----- .../datastrato/gravitino/listener/TopicEventDispatcher.java | 5 ----- 12 files changed, 6 insertions(+), 51 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java index 3d92101017a..e61838e9556 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaDispatcher.java @@ -5,7 +5,6 @@ package com.datastrato.gravitino.catalog; -import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.connector.SupportsSchemas; /** @@ -14,6 +13,4 @@ * to dispatching or handling schema-related events or actions that are not covered by the standard * {@code SupportsSchemas} operations. */ -public interface SchemaDispatcher extends SupportsSchemas { - boolean importSchema(NameIdentifier identifier); -} +public interface SchemaDispatcher extends SupportsSchemas {} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java index 32d1be56d76..9c004989e80 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaNormalizeDispatcher.java @@ -71,12 +71,6 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty applyCaseSensitive(ident, Capability.Scope.SCHEMA, dispatcher), cascade); } - @Override - public boolean importSchema(NameIdentifier identifier) { - return dispatcher.importSchema( - applyCaseSensitive(identifier, Capability.Scope.SCHEMA, dispatcher)); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.SCHEMA, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index b5ab82276a2..2f12b347da1 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -309,8 +309,7 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty : droppedFromCatalog; } - @Override - public boolean importSchema(NameIdentifier identifier) { + private boolean importSchema(NameIdentifier identifier) { EntityCombinedSchema combinedSchema = loadCombinedSchema(identifier); if (combinedSchema.imported()) { return false; diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java index 96a604d2a4f..7b54ccd5794 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableDispatcher.java @@ -5,7 +5,6 @@ package com.datastrato.gravitino.catalog; -import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.rel.TableCatalog; /** @@ -14,6 +13,4 @@ * dispatching or handling table-related events or actions that are not covered by the standard * {@code TableCatalog} operations. */ -public interface TableDispatcher extends TableCatalog { - boolean importTable(NameIdentifier identifier); -} +public interface TableDispatcher extends TableCatalog {} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java index aa1730fcfe0..ac7da6cbc55 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java @@ -99,12 +99,6 @@ public boolean tableExists(NameIdentifier ident) { return dispatcher.tableExists(applyCaseSensitive(ident, Capability.Scope.TABLE, dispatcher)); } - @Override - public boolean importTable(NameIdentifier identifier) { - return dispatcher.importTable( - applyCaseSensitive(identifier, Capability.Scope.TABLE, dispatcher)); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.TABLE, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 314781fafd6..128ca21eb25 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -366,8 +366,7 @@ public boolean purgeTable(NameIdentifier ident) throws UnsupportedOperationExcep : droppedFromCatalog; } - @Override - public boolean importTable(NameIdentifier identifier) { + private boolean importTable(NameIdentifier identifier) { EntityCombinedTable combinedTable = loadCombinedTable(identifier); if (combinedTable.imported()) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java index 7d96c5abf95..131a600c621 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicDispatcher.java @@ -5,7 +5,6 @@ package com.datastrato.gravitino.catalog; -import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.messaging.TopicCatalog; /** @@ -14,6 +13,4 @@ * dispatching or handling topic-related events or actions that are not covered by the standard * {@code TopicCatalog} operations. */ -public interface TopicDispatcher extends TopicCatalog { - boolean importTopic(NameIdentifier identifier); -} +public interface TopicDispatcher extends TopicCatalog {} diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java index de05fc04cc4..aa372b623c1 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicNormalizeDispatcher.java @@ -72,12 +72,6 @@ public boolean dropTopic(NameIdentifier ident) { return dispatcher.dropTopic(applyCaseSensitive(ident, Capability.Scope.TOPIC, dispatcher)); } - @Override - public boolean importTopic(NameIdentifier identifier) { - return dispatcher.importTopic( - applyCaseSensitive(identifier, Capability.Scope.TOPIC, dispatcher)); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = dispatcher.getCatalogCapability(ident); return applyCapabilities(ident, Capability.Scope.TOPIC, capability); diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index 998421d3cbf..5401d341d3e 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -268,8 +268,7 @@ public boolean dropTopic(NameIdentifier ident) { : droppedFromCatalog; } - @Override - public boolean importTopic(NameIdentifier identifier) { + private boolean importTopic(NameIdentifier identifier) { EntityCombinedTopic combinedTopic = loadCombinedTopic(identifier); diff --git a/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java index 21ec9a704db..50c6df4ea18 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/SchemaEventDispatcher.java @@ -130,9 +130,4 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty throw e; } } - - @Override - public boolean importSchema(NameIdentifier identifier) { - return dispatcher.importSchema(identifier); - } } diff --git a/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java index e1a78c35840..deacbba2094 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/TableEventDispatcher.java @@ -169,9 +169,4 @@ public boolean purgeTable(NameIdentifier ident) { public boolean tableExists(NameIdentifier ident) { return dispatcher.tableExists(ident); } - - @Override - public boolean importTable(NameIdentifier identifier) { - return dispatcher.importTable(identifier); - } } diff --git a/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java b/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java index 61777703064..dd628a534d2 100644 --- a/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/listener/TopicEventDispatcher.java @@ -128,9 +128,4 @@ public Topic createTopic( throw e; } } - - @Override - public boolean importTopic(NameIdentifier identifier) { - return dispatcher.importTopic(identifier); - } } From d5ca7e7997b15355b27e580b4eb3a67abab1cfac Mon Sep 17 00:00:00 2001 From: Heng Qin Date: Wed, 5 Jun 2024 11:31:16 +0800 Subject: [PATCH 03/15] Avoid null pointer exception --- .../datastrato/gravitino/catalog/SchemaOperationDispatcher.java | 2 +- .../datastrato/gravitino/catalog/TableOperationDispatcher.java | 2 +- .../datastrato/gravitino/catalog/TopicOperationDispatcher.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index 2f12b347da1..178b0f921fc 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -164,7 +164,7 @@ public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException { EntityCombinedSchema schema = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedSchema(ident)); - if (schema.imported()) { + if (schema == null || schema.imported()) { return schema; } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 128ca21eb25..52650303f33 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -84,7 +84,7 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { EntityCombinedTable table = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTable(ident)); - if (table.imported()) { + if (table == null || table.imported()) { return table; } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index 5401d341d3e..c0a43f73fe0 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -75,7 +75,7 @@ public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { EntityCombinedTopic topic = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTopic(ident)); - if (topic.imported()) { + if (topic == null || topic.imported()) { return topic; } From c831685afa6b8f5bb16c51ea5f224358224be93d Mon Sep 17 00:00:00 2001 From: Heng Qin Date: Wed, 5 Jun 2024 22:06:25 +0800 Subject: [PATCH 04/15] check schema --- .../datastrato/gravitino/GravitinoEnv.java | 50 ++++++++ .../catalog/SchemaOperationDispatcher.java | 11 +- .../catalog/TableOperationDispatcher.java | 17 +-- .../catalog/TopicOperationDispatcher.java | 14 ++- .../TestSchemaOperationDispatcher.java | 69 +++++++++++ .../catalog/TestTableOperationDispatcher.java | 16 +++ .../catalog/TestTopicOperationDispatcher.java | 19 +++ .../server/web/rest/RoleOperations.java | 49 ++++---- .../server/web/rest/TestRoleOperations.java | 115 +++++++++++++++++- 9 files changed, 310 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java index c48e0fcf02c..447717d77cb 100644 --- a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java +++ b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java @@ -121,6 +121,16 @@ public void setAccessControlManager(AccessControlManager accessControlManager) { this.accessControlManager = accessControlManager; } + /** + * This method is used for testing purposes only to set the metalake dispatcher for test. + * + * @param metalakeDispatcher The metalake dispatcher to be set. + */ + @VisibleForTesting + public void setMetalakeDispatcher(MetalakeDispatcher metalakeDispatcher) { + this.metalakeDispatcher = metalakeDispatcher; + } + /** * This method is used for testing purposes only to set the access manager for test in package * `com.datastrato.gravitino.server.web.rest`. @@ -132,6 +142,46 @@ public void setCatalogDispatcher(CatalogDispatcher catalogDispatcher) { this.catalogDispatcher = catalogDispatcher; } + /** + * This method is used for testing purposes only to set the schema dispatcher for test. + * + * @param schemaDispatcher The schema dispatcher to be set. + */ + @VisibleForTesting + public void setSchemaDispatcher(SchemaDispatcher schemaDispatcher) { + this.schemaDispatcher = schemaDispatcher; + } + + /** + * This method is used for testing purposes only to set the table dispatcher for test. + * + * @param tableDispatcher The table dispatcher to be set. + */ + @VisibleForTesting + public void setTableDispatcher(TableDispatcher tableDispatcher) { + this.tableDispatcher = tableDispatcher; + } + + /** + * This method is used for testing purposes only to set the topic dispatcher for test. + * + * @param topicDispatcher The topic dispatcher to be set. + */ + @VisibleForTesting + public void setTopicDispatcher(TopicDispatcher topicDispatcher) { + this.topicDispatcher = topicDispatcher; + } + + /** + * This method is used for testing purposes only to set the filset dispatcher for test. + * + * @param filesetDispatcher The fileset dispatcher to be set. + */ + @VisibleForTesting + public void setFilesetDispatcher(FilesetDispatcher filesetDispatcher) { + this.filesetDispatcher = filesetDispatcher; + } + /** * This method is used for testing purposes only to set the entity store for test in package * `com.datastrato.gravitino.authorization`. diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index 178b0f921fc..cbf5e279975 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -214,8 +214,7 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())) - .withImported(true); + alteredSchema.properties())); } StringIdentifier stringId = getStringIdFromProperties(alteredSchema.properties()); @@ -226,8 +225,7 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())) - .withImported(isEntityExist(ident)); + alteredSchema.properties())); } SchemaEntity updatedSchemaEntity = @@ -255,15 +253,12 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) "UPDATE", stringId.id()); - boolean imported = updatedSchemaEntity != null; - return EntityCombinedSchema.of(alteredSchema, updatedSchemaEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::schemaPropertiesMetadata, - alteredSchema.properties())) - .withImported(imported); + alteredSchema.properties())); } /** diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 52650303f33..836c3aa7237 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -10,6 +10,7 @@ import static com.datastrato.gravitino.rel.expressions.transforms.Transforms.EMPTY_TRANSFORM; import com.datastrato.gravitino.EntityStore; +import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.StringIdentifier; @@ -88,8 +89,12 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { return table; } - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTable(ident)); + if (GravitinoEnv.getInstance() + .schemaDispatcher() + .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTable(ident)); + } return table; } @@ -231,8 +236,7 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) getHiddenPropertyNames( getCatalogIdentifier(ident), HasPropertyMetadata::tablePropertiesMetadata, - alteredTable.properties())) - .withImported(isEntityExist(ident)); + alteredTable.properties())); } TableEntity updatedTableEntity = @@ -267,15 +271,12 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) "UPDATE", stringId.id()); - boolean imported = updatedTableEntity != null; - return EntityCombinedTable.of(alteredTable, updatedTableEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( getCatalogIdentifier(ident), HasPropertyMetadata::tablePropertiesMetadata, - alteredTable.properties())) - .withImported(imported); + alteredTable.properties())); } /** diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index c0a43f73fe0..1bc69760633 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -9,6 +9,7 @@ import static com.datastrato.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForCreate; import com.datastrato.gravitino.EntityStore; +import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.StringIdentifier; @@ -79,8 +80,12 @@ public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { return topic; } - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTopic(ident)); + if (GravitinoEnv.getInstance() + .schemaDispatcher() + .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTopic(ident)); + } return topic; } @@ -219,15 +224,12 @@ public Topic alterTopic(NameIdentifier ident, TopicChange... changes) "UPDATE", getStringIdFromProperties(alteredTopic.properties()).id()); - boolean imported = updatedTopicEntity != null; - return EntityCombinedTopic.of(alteredTopic, updatedTopicEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, - alteredTopic.properties())) - .withImported(imported); + alteredTopic.properties())); } /** diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java index b8e8e303fa7..1cf998e565c 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java @@ -16,6 +16,7 @@ import com.datastrato.gravitino.Config; import com.datastrato.gravitino.Configs; +import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; @@ -118,6 +119,74 @@ public void testCreateAndListSchemas() throws IOException { Assertions.assertEquals("test", schema2.auditInfo().creator()); } + @Test + public void testCreateAndLoadSchema() throws IOException { + NameIdentifier schemaIdent = NameIdentifier.of(metalake, catalog, "schema20"); + Map props = ImmutableMap.of("k1", "v1", "k2", "v2"); + Schema schema = dispatcher.createSchema(schemaIdent, "comment", props); + Assertions.assertEquals("schema20", schema.name()); + Assertions.assertEquals("comment", schema.comment()); + testProperties(props, schema.properties()); + + Schema loadedSchema = dispatcher.loadSchema(schemaIdent); + Assertions.assertEquals(loadedSchema.name(), schema.name()); + Assertions.assertEquals(loadedSchema.comment(), schema.comment()); + testProperties(loadedSchema.properties(), schema.properties()); + // Audit info is gotten from the entity store + Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, loadedSchema.auditInfo().creator()); + + // Case 2: Test if the schema is not found in entity store + doThrow(new NoSuchEntityException("mock error")).when(entityStore).get(any(), any(), any()); + entityStore.delete(schemaIdent, Entity.EntityType.SCHEMA); + Schema loadedSchema1 = dispatcher.loadSchema(schemaIdent); + Assertions.assertEquals(schema.name(), loadedSchema1.name()); + Assertions.assertEquals(schema.comment(), loadedSchema1.comment()); + testProperties(props, loadedSchema1.properties()); + // Succeed to import the topic entity + Assertions.assertTrue(entityStore.exists(schemaIdent, SCHEMA)); + + // Audit info is gotten from catalog, not from the entity store + Assertions.assertEquals("test", loadedSchema1.auditInfo().creator()); + + // Case 3: Test if entity store is failed to get the schema entity + reset(entityStore); + doThrow(new IOException()).when(entityStore).get(any(), any(), any()); + entityStore.delete(schemaIdent, Entity.EntityType.SCHEMA); + Schema loadedSchema2 = dispatcher.loadSchema(schemaIdent); + // Succeed to import the topic entity + Assertions.assertTrue(entityStore.exists(schemaIdent, SCHEMA)); + Assertions.assertEquals(schema.name(), loadedSchema2.name()); + Assertions.assertEquals(schema.comment(), loadedSchema2.comment()); + testProperties(props, loadedSchema2.properties()); + // Audit info is gotten from catalog, not from the entity store + Assertions.assertEquals("test", loadedSchema2.auditInfo().creator()); + + // Case 4: Test if the fetched schema entity is matched. + reset(entityStore); + SchemaEntity unmatchedEntity = + SchemaEntity.builder() + .withId(1L) + .withName("schema21") + .withNamespace(Namespace.of(metalake, catalog)) + .withAuditInfo( + AuditInfo.builder() + .withCreator(AuthConstants.ANONYMOUS_USER) + .withCreateTime(Instant.now()) + .build()) + .build(); + doReturn(unmatchedEntity).when(entityStore).get(any(), any(), any()); + Schema loadedSchema3 = dispatcher.loadSchema(schemaIdent); + // Succeed to import the schema entity + reset(entityStore); + SchemaEntity schemaEntity = entityStore.get(schemaIdent, SCHEMA, SchemaEntity.class); + Assertions.assertEquals("test", schemaEntity.auditInfo().creator()); + Assertions.assertEquals(schema.name(), loadedSchema3.name()); + Assertions.assertEquals(schema.comment(), loadedSchema3.comment()); + testProperties(props, loadedSchema3.properties()); + // Audit info is gotten from catalog, not from the entity store + Assertions.assertEquals("test", loadedSchema3.auditInfo().creator()); + } + @Test public void testCreateAndAlterSchema() throws IOException { NameIdentifier schemaIdent = NameIdentifier.of(metalake, catalog, "schema21"); diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java index f3c1324b671..a37e7e5acdb 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java @@ -7,6 +7,7 @@ import static com.datastrato.gravitino.Configs.TREE_LOCK_CLEAN_INTERVAL; import static com.datastrato.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY; import static com.datastrato.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY; +import static com.datastrato.gravitino.Entity.EntityType.SCHEMA; import static com.datastrato.gravitino.Entity.EntityType.TABLE; import static com.datastrato.gravitino.StringIdentifier.ID_KEY; import static com.datastrato.gravitino.TestBasePropertiesMetadata.COMMENT_KEY; @@ -59,6 +60,7 @@ public static void initialize() throws IOException { doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + GravitinoEnv.getInstance().setSchemaDispatcher(schemaOperationDispatcher); } @Test @@ -167,15 +169,25 @@ public void testCreateAndLoadTable() throws IOException { // Case 2: Test if the table entity is not found in the entity store reset(entityStore); + entityStore.delete(tableIdent1, TABLE); + entityStore.delete(NameIdentifier.of(tableNs.levels()), SCHEMA); doThrow(new NoSuchEntityException("")).when(entityStore).get(any(), any(), any()); Table loadedTable2 = tableOperationDispatcher.loadTable(tableIdent1); + // Succeed to import the topic entity + Assertions.assertTrue(entityStore.exists(NameIdentifier.of(tableNs.levels()), SCHEMA)); + Assertions.assertTrue(entityStore.exists(tableIdent1, TABLE)); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTable2.auditInfo().creator()); // Case 3: Test if the entity store is failed to get the table entity reset(entityStore); + entityStore.delete(tableIdent1, TABLE); + entityStore.delete(NameIdentifier.of(tableNs.levels()), SCHEMA); doThrow(new IOException()).when(entityStore).get(any(), any(), any()); Table loadedTable3 = tableOperationDispatcher.loadTable(tableIdent1); + // Succeed to import the topic entity + Assertions.assertTrue(entityStore.exists(NameIdentifier.of(tableNs.levels()), SCHEMA)); + Assertions.assertTrue(entityStore.exists(tableIdent1, TABLE)); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTable3.auditInfo().creator()); @@ -191,6 +203,10 @@ public void testCreateAndLoadTable() throws IOException { .build(); doReturn(tableEntity).when(entityStore).get(any(), any(), any()); Table loadedTable4 = tableOperationDispatcher.loadTable(tableIdent1); + // Succeed to import the topic entity + reset(entityStore); + TableEntity tableImportedEntity = entityStore.get(tableIdent1, TABLE, TableEntity.class); + Assertions.assertEquals("test", tableImportedEntity.auditInfo().creator()); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTable4.auditInfo().creator()); } diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java index ccf55f1a19d..0371e251d69 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java @@ -17,6 +17,7 @@ import static org.mockito.Mockito.reset; import com.datastrato.gravitino.Config; +import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; @@ -52,6 +53,7 @@ public static void initialize() throws IOException { doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + GravitinoEnv.getInstance().setSchemaDispatcher(schemaOperationDispatcher); } @Test @@ -103,15 +105,27 @@ public void testCreateAndLoadTopic() throws IOException { // Case 2: Test if the topic entity is not found in the entity store reset(entityStore); + entityStore.delete(topicIdent1, Entity.EntityType.TOPIC); + entityStore.delete(NameIdentifier.of(topicNs.levels()), Entity.EntityType.SCHEMA); doThrow(new NoSuchEntityException("")).when(entityStore).get(any(), any(), any()); Topic loadedTopic2 = topicOperationDispatcher.loadTopic(topicIdent1); + // Succeed to import the topic entity + Assertions.assertTrue(entityStore.exists(topicIdent1, Entity.EntityType.TOPIC)); + Assertions.assertTrue( + entityStore.exists(NameIdentifier.of(topicNs.levels()), Entity.EntityType.SCHEMA)); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTopic2.auditInfo().creator()); // Case 3: Test if the entity store is failed to get the topic entity reset(entityStore); + entityStore.delete(topicIdent1, Entity.EntityType.TOPIC); + entityStore.delete(NameIdentifier.of(topicNs.levels()), Entity.EntityType.SCHEMA); doThrow(new IOException()).when(entityStore).get(any(), any(), any()); Topic loadedTopic3 = topicOperationDispatcher.loadTopic(topicIdent1); + // Succeed to import the topic entity + Assertions.assertTrue( + entityStore.exists(NameIdentifier.of(topicNs.levels()), Entity.EntityType.SCHEMA)); + Assertions.assertTrue(entityStore.exists(topicIdent1, Entity.EntityType.TOPIC)); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTopic3.auditInfo().creator()); @@ -127,6 +141,11 @@ public void testCreateAndLoadTopic() throws IOException { .build(); doReturn(unmatchedEntity).when(entityStore).get(any(), any(), any()); Topic loadedTopic4 = topicOperationDispatcher.loadTopic(topicIdent1); + // Succeed to import the topic entity + reset(entityStore); + TopicEntity topicEntity = + entityStore.get(topicIdent1, Entity.EntityType.TOPIC, TopicEntity.class); + Assertions.assertEquals("test", topicEntity.auditInfo().creator()); // Audit info is gotten from the catalog, not from the entity store Assertions.assertEquals("test", loadedTopic4.auditInfo().creator()); } diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java index bfc84ee0936..e8575c56341 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java @@ -14,9 +14,6 @@ import com.datastrato.gravitino.authorization.Privileges; import com.datastrato.gravitino.authorization.SecurableObject; import com.datastrato.gravitino.authorization.SecurableObjects; -import com.datastrato.gravitino.catalog.SchemaDispatcher; -import com.datastrato.gravitino.catalog.TableDispatcher; -import com.datastrato.gravitino.catalog.TopicDispatcher; import com.datastrato.gravitino.dto.authorization.SecurableObjectDTO; import com.datastrato.gravitino.dto.requests.RoleCreateRequest; import com.datastrato.gravitino.dto.responses.DeleteResponse; @@ -43,6 +40,7 @@ @Path("/metalakes/{metalake}/roles") public class RoleOperations { private static final Logger LOG = LoggerFactory.getLogger(RoleOperations.class); + private static final String ALL_METALAKES = "*"; private final AccessControlManager accessControlManager; @@ -86,7 +84,7 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq "The size of securable objects must be 1"); for (SecurableObjectDTO object : request.getSecurableObjects()) { - checkSecurableObjects(metalake, object); + checkSecurableObject(metalake, object); } return Utils.doAs( @@ -146,11 +144,15 @@ public Response deleteRole( } // Check every securable object whether exists and is imported. - private void checkSecurableObjects(String metalake, SecurableObjectDTO object) { + static void checkSecurableObject(String metalake, SecurableObjectDTO object) { NameIdentifier identifier; // Securable object ignores the metalake namespace, so we should add it back. if (object.type() == MetadataObject.Type.METALAKE) { + // All metalakes don't need to check the securable object whether exists. + if (object.name().equals(ALL_METALAKES)) { + return; + } identifier = NameIdentifier.parse(object.fullName()); } else { identifier = NameIdentifier.parse(String.format("%s.%s", metalake, object.fullName())); @@ -169,20 +171,21 @@ private void checkSecurableObjects(String metalake, SecurableObjectDTO object) { } break; + case CATALOG: if (!GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier)) { throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); } break; + case SCHEMA: - { - SchemaDispatcher dispatcher = GravitinoEnv.getInstance().schemaDispatcher(); - if (!dispatcher.schemaExists(identifier)) { - throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); - } - break; + if (!GravitinoEnv.getInstance().schemaDispatcher().schemaExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); } + + break; + case FILESET: if (!GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier)) { throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); @@ -190,23 +193,19 @@ private void checkSecurableObjects(String metalake, SecurableObjectDTO object) { break; case TABLE: - { - TableDispatcher dispatcher = GravitinoEnv.getInstance().tableDispatcher(); - if (!dispatcher.tableExists(identifier)) { - throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); - } - - break; + if (!GravitinoEnv.getInstance().tableDispatcher().tableExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); } - case TOPIC: - { - TopicDispatcher dispatcher = GravitinoEnv.getInstance().topicDispatcher(); - if (!dispatcher.topicExists(identifier)) { - throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); - } - break; + break; + + case TOPIC: + if (!GravitinoEnv.getInstance().topicDispatcher().topicExists(identifier)) { + throw new IllegalArgumentException(String.format(existErrMsg, object.fullName())); } + + break; + default: throw new IllegalArgumentException( String.format("Doesn't support the type %s", object.type())); diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java index e8cdced23e6..a677d226383 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java @@ -20,6 +20,10 @@ import com.datastrato.gravitino.authorization.SecurableObject; import com.datastrato.gravitino.authorization.SecurableObjects; import com.datastrato.gravitino.catalog.CatalogDispatcher; +import com.datastrato.gravitino.catalog.FilesetDispatcher; +import com.datastrato.gravitino.catalog.SchemaDispatcher; +import com.datastrato.gravitino.catalog.TableDispatcher; +import com.datastrato.gravitino.catalog.TopicDispatcher; import com.datastrato.gravitino.dto.authorization.RoleDTO; import com.datastrato.gravitino.dto.authorization.SecurableObjectDTO; import com.datastrato.gravitino.dto.requests.RoleCreateRequest; @@ -34,6 +38,7 @@ import com.datastrato.gravitino.lock.LockManager; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.RoleEntity; +import com.datastrato.gravitino.metalake.MetalakeDispatcher; import com.datastrato.gravitino.rest.RESTUtils; import com.google.common.collect.Lists; import java.io.IOException; @@ -56,7 +61,12 @@ public class TestRoleOperations extends JerseyTest { private static final AccessControlManager manager = mock(AccessControlManager.class); - private static final CatalogDispatcher dispatcher = mock(CatalogDispatcher.class); + private static final MetalakeDispatcher metalakeDispatcher = mock(MetalakeDispatcher.class); + private static final CatalogDispatcher catalogDispatcher = mock(CatalogDispatcher.class); + private static final SchemaDispatcher schemaDispatcher = mock(SchemaDispatcher.class); + private static final TableDispatcher tableDispatcher = mock(TableDispatcher.class); + private static final TopicDispatcher topicDispatcher = mock(TopicDispatcher.class); + private static final FilesetDispatcher filesetDispatcher = mock(FilesetDispatcher.class); private static class MockServletRequestFactory extends ServletRequestFactoryBase { @Override @@ -75,7 +85,12 @@ public static void setup() { Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); GravitinoEnv.getInstance().setLockManager(new LockManager(config)); GravitinoEnv.getInstance().setAccessControlManager(manager); - GravitinoEnv.getInstance().setCatalogDispatcher(dispatcher); + GravitinoEnv.getInstance().setMetalakeDispatcher(metalakeDispatcher); + GravitinoEnv.getInstance().setCatalogDispatcher(catalogDispatcher); + GravitinoEnv.getInstance().setSchemaDispatcher(schemaDispatcher); + GravitinoEnv.getInstance().setTableDispatcher(tableDispatcher); + GravitinoEnv.getInstance().setTopicDispatcher(topicDispatcher); + GravitinoEnv.getInstance().setFilesetDispatcher(filesetDispatcher); } @Override @@ -113,7 +128,7 @@ public void testCreateRole() { Role role = buildRole("role1"); when(manager.createRole(any(), any(), any(), any())).thenReturn(role); - when(dispatcher.catalogExists(any())).thenReturn(true); + when(catalogDispatcher.catalogExists(any())).thenReturn(true); Response resp = target("/metalakes/metalake1/roles") @@ -141,7 +156,20 @@ public void testCreateRole() { Privileges.UseCatalog.allow().condition(), roleDTO.securableObjects().get(0).privileges().get(0).condition()); + // Test to a catalog which doesn't exist + when(catalogDispatcher.catalogExists(any())).thenReturn(false); + Response respNotExist = + target("/metalakes/metalake1/roles") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), respNotExist.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, respNotExist.getMediaType()); + ErrorResponse notExistResponse = respNotExist.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE, notExistResponse.getCode()); + // Test to throw NoSuchMetalakeException + when(catalogDispatcher.catalogExists(any())).thenReturn(true); doThrow(new NoSuchMetalakeException("mock error")) .when(manager) .createRole(any(), any(), any(), any()); @@ -323,4 +351,85 @@ public void testDeleteRole() { Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse.getCode()); Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse.getType()); } + + @Test + public void testCheckSecurableObjects() { + // check all metalakes + SecurableObject allMetalake = + SecurableObjects.ofAllMetalakes(Lists.newArrayList(Privileges.UseMetalake.allow())); + when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(allMetalake))); + when(metalakeDispatcher.metalakeExists(any())).thenReturn(false); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(allMetalake))); + + // check the metalake + SecurableObject metalake = + SecurableObjects.ofMetalake("metalake", Lists.newArrayList(Privileges.UseMetalake.allow())); + when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(metalake))); + when(metalakeDispatcher.metalakeExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(metalake))); + + // check the catalog + SecurableObject catalog = + SecurableObjects.ofCatalog("catalog", Lists.newArrayList(Privileges.UseCatalog.allow())); + when(catalogDispatcher.catalogExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(catalog))); + when(catalogDispatcher.catalogExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(catalog))); + + // check the schema + SecurableObject schema = + SecurableObjects.ofSchema( + catalog, "schema", Lists.newArrayList(Privileges.UseSchema.allow())); + when(schemaDispatcher.schemaExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(schema))); + when(schemaDispatcher.schemaExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(schema))); + + // check the table + SecurableObject table = + SecurableObjects.ofTable(schema, "table", Lists.newArrayList(Privileges.ReadTable.allow())); + when(tableDispatcher.tableExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(table))); + when(tableDispatcher.tableExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(table))); + + // check the topic + SecurableObject topic = + SecurableObjects.ofTopic(schema, "topic", Lists.newArrayList(Privileges.ReadTopic.allow())); + when(topicDispatcher.topicExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(topic))); + when(topicDispatcher.topicExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(topic))); + + // check the fileset + SecurableObject fileset = + SecurableObjects.ofFileset( + schema, "fileset", Lists.newArrayList(Privileges.ReadFileset.allow())); + when(filesetDispatcher.filesetExists(any())).thenReturn(true); + Assertions.assertDoesNotThrow( + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(fileset))); + when(filesetDispatcher.filesetExists(any())).thenReturn(false); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RoleOperations.checkSecurableObject("metalake", DTOConverters.toDTO(fileset))); + } } From 1b8f375511733f7fe753a9b033b7728fbe48c37b Mon Sep 17 00:00:00 2001 From: Rory Date: Tue, 11 Jun 2024 14:20:31 +0800 Subject: [PATCH 05/15] Address comments --- .../datastrato/gravitino/GravitinoEnv.java | 72 ------------------- .../TestAccessControlManager.java | 6 +- ...estAccessControlManagerForPermissions.java | 6 +- .../catalog/TestTableNormalizeDispatcher.java | 2 +- .../catalog/TestTableOperationDispatcher.java | 8 ++- .../catalog/TestTopicNormalizeDispatcher.java | 2 +- .../catalog/TestTopicOperationDispatcher.java | 8 ++- .../server/web/rest/RoleOperations.java | 4 +- .../server/web/rest/TestGroupOperations.java | 7 +- .../web/rest/TestMetalakeAdminOperations.java | 7 +- .../web/rest/TestPermissionOperations.java | 7 +- .../server/web/rest/TestRoleOperations.java | 22 +++--- .../server/web/rest/TestUserOperations.java | 7 +- 13 files changed, 51 insertions(+), 107 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java index 447717d77cb..3a98b48fc27 100644 --- a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java +++ b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java @@ -121,78 +121,6 @@ public void setAccessControlManager(AccessControlManager accessControlManager) { this.accessControlManager = accessControlManager; } - /** - * This method is used for testing purposes only to set the metalake dispatcher for test. - * - * @param metalakeDispatcher The metalake dispatcher to be set. - */ - @VisibleForTesting - public void setMetalakeDispatcher(MetalakeDispatcher metalakeDispatcher) { - this.metalakeDispatcher = metalakeDispatcher; - } - - /** - * This method is used for testing purposes only to set the access manager for test in package - * `com.datastrato.gravitino.server.web.rest`. - * - * @param catalogDispatcher The catalog dispatcher to be set. - */ - @VisibleForTesting - public void setCatalogDispatcher(CatalogDispatcher catalogDispatcher) { - this.catalogDispatcher = catalogDispatcher; - } - - /** - * This method is used for testing purposes only to set the schema dispatcher for test. - * - * @param schemaDispatcher The schema dispatcher to be set. - */ - @VisibleForTesting - public void setSchemaDispatcher(SchemaDispatcher schemaDispatcher) { - this.schemaDispatcher = schemaDispatcher; - } - - /** - * This method is used for testing purposes only to set the table dispatcher for test. - * - * @param tableDispatcher The table dispatcher to be set. - */ - @VisibleForTesting - public void setTableDispatcher(TableDispatcher tableDispatcher) { - this.tableDispatcher = tableDispatcher; - } - - /** - * This method is used for testing purposes only to set the topic dispatcher for test. - * - * @param topicDispatcher The topic dispatcher to be set. - */ - @VisibleForTesting - public void setTopicDispatcher(TopicDispatcher topicDispatcher) { - this.topicDispatcher = topicDispatcher; - } - - /** - * This method is used for testing purposes only to set the filset dispatcher for test. - * - * @param filesetDispatcher The fileset dispatcher to be set. - */ - @VisibleForTesting - public void setFilesetDispatcher(FilesetDispatcher filesetDispatcher) { - this.filesetDispatcher = filesetDispatcher; - } - - /** - * This method is used for testing purposes only to set the entity store for test in package - * `com.datastrato.gravitino.authorization`. - * - * @param entityStore The entity store to be set. - */ - @VisibleForTesting - public void setEntityStore(EntityStore entityStore) { - this.entityStore = entityStore; - } - /** * Initialize the Gravitino environment. * diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManager.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManager.java index a78ea934d1b..c7f5b6058dc 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManager.java +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManager.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.time.Instant; import java.util.Map; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; @@ -63,8 +64,9 @@ public static void setUp() throws Exception { entityStore.put(metalakeEntity, true); accessControlManager = new AccessControlManager(entityStore, new RandomIdGenerator(), config); - GravitinoEnv.getInstance().setEntityStore(entityStore); - GravitinoEnv.getInstance().setAccessControlManager(accessControlManager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "entityStore", entityStore, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "accessControlManager", accessControlManager, true); } @AfterAll diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java index f67437dfe6d..0a90ecf0fa8 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.time.Instant; import java.util.List; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; @@ -108,8 +109,9 @@ public static void setUp() throws Exception { accessControlManager = new AccessControlManager(entityStore, new RandomIdGenerator(), config); - GravitinoEnv.getInstance().setEntityStore(entityStore); - GravitinoEnv.getInstance().setAccessControlManager(accessControlManager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "entityStore", entityStore, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "accessControlManager", accessControlManager, true); } @AfterAll diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableNormalizeDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableNormalizeDispatcher.java index 906a987c500..a26ad08354d 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableNormalizeDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableNormalizeDispatcher.java @@ -37,7 +37,7 @@ public class TestTableNormalizeDispatcher extends TestOperationDispatcher { private static SchemaNormalizeDispatcher schemaNormalizeDispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { TestTableOperationDispatcher.initialize(); tableNormalizeDispatcher = new TableNormalizeDispatcher(TestTableOperationDispatcher.tableOperationDispatcher); diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java index a37e7e5acdb..f2d9678227d 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTableOperationDispatcher.java @@ -40,6 +40,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -49,7 +50,7 @@ public class TestTableOperationDispatcher extends TestOperationDispatcher { static SchemaOperationDispatcher schemaOperationDispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { schemaOperationDispatcher = new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); tableOperationDispatcher = @@ -59,8 +60,9 @@ public static void initialize() throws IOException { doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setSchemaDispatcher(schemaOperationDispatcher); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "schemaDispatcher", schemaOperationDispatcher, true); } @Test diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicNormalizeDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicNormalizeDispatcher.java index 3772876d81a..d15c18a9ebf 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicNormalizeDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicNormalizeDispatcher.java @@ -22,7 +22,7 @@ public class TestTopicNormalizeDispatcher extends TestOperationDispatcher { private static SchemaNormalizeDispatcher schemaNormalizeDispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { TestTopicOperationDispatcher.initialize(); schemaNormalizeDispatcher = new SchemaNormalizeDispatcher(TestTopicOperationDispatcher.schemaOperationDispatcher); diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java index 0371e251d69..4c5d00410b5 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestTopicOperationDispatcher.java @@ -32,6 +32,7 @@ import java.io.IOException; import java.time.Instant; import java.util.Map; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -42,7 +43,7 @@ public class TestTopicOperationDispatcher extends TestOperationDispatcher { static TopicOperationDispatcher topicOperationDispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { schemaOperationDispatcher = new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); topicOperationDispatcher = @@ -52,8 +53,9 @@ public static void initialize() throws IOException { doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setSchemaDispatcher(schemaOperationDispatcher); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "schemaDispatcher", schemaOperationDispatcher, true); } @Test diff --git a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java index 02227d3252f..39d35b5d1d3 100644 --- a/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java +++ b/server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java @@ -6,6 +6,7 @@ import com.codahale.metrics.annotation.ResponseMetered; import com.codahale.metrics.annotation.Timed; +import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.MetadataObject; import com.datastrato.gravitino.NameIdentifier; @@ -41,7 +42,6 @@ @Path("/metalakes/{metalake}/roles") public class RoleOperations { private static final Logger LOG = LoggerFactory.getLogger(RoleOperations.class); - private static final String ALL_METALAKES = "*"; private final AccessControlManager accessControlManager; @@ -149,7 +149,7 @@ static void checkSecurableObject(String metalake, SecurableObjectDTO object) { // Securable object ignores the metalake namespace, so we should add it back. if (object.type() == MetadataObject.Type.METALAKE) { // All metalakes don't need to check the securable object whether exists. - if (object.name().equals(ALL_METALAKES)) { + if (object.name().equals(Entity.SECURABLE_ENTITY_RESERVED_NAME)) { return; } identifier = NameIdentifier.parse(object.fullName()); diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestGroupOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestGroupOperations.java index ecba282167d..4b232ca1016 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestGroupOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestGroupOperations.java @@ -37,6 +37,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -60,13 +61,13 @@ public HttpServletRequest get() { } @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setAccessControlManager(manager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlManager", manager, true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeAdminOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeAdminOperations.java index 8437626dc19..2ffbad2edfc 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeAdminOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeAdminOperations.java @@ -32,6 +32,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -55,13 +56,13 @@ public HttpServletRequest get() { } @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = Mockito.mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setAccessControlManager(manager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlManager", manager, true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPermissionOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPermissionOperations.java index 9e8ee453b7c..501ceccaeb3 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPermissionOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPermissionOperations.java @@ -39,6 +39,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -62,13 +63,13 @@ public HttpServletRequest get() { } @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setAccessControlManager(manager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlManager", manager, true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java index c1b0e8bdab0..3e0194e36f2 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestRoleOperations.java @@ -49,6 +49,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -78,19 +79,22 @@ public HttpServletRequest get() { } @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setAccessControlManager(manager); - GravitinoEnv.getInstance().setMetalakeDispatcher(metalakeDispatcher); - GravitinoEnv.getInstance().setCatalogDispatcher(catalogDispatcher); - GravitinoEnv.getInstance().setSchemaDispatcher(schemaDispatcher); - GravitinoEnv.getInstance().setTableDispatcher(tableDispatcher); - GravitinoEnv.getInstance().setTopicDispatcher(topicDispatcher); - GravitinoEnv.getInstance().setFilesetDispatcher(filesetDispatcher); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlManager", manager, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogDispatcher", catalogDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "schemaDispatcher", schemaDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "tableDispatcher", tableDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "topicDispatcher", topicDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "filesetDispatcher", filesetDispatcher, true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestUserOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestUserOperations.java index 71e00d6f33a..8d39f4b44b0 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestUserOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestUserOperations.java @@ -37,6 +37,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -60,13 +61,13 @@ public HttpServletRequest get() { } @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); - GravitinoEnv.getInstance().setAccessControlManager(manager); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlManager", manager, true); } @Override From b5d5cd2a33e5b7838b2f7eb125187b911883a7ab Mon Sep 17 00:00:00 2001 From: Rory Date: Tue, 11 Jun 2024 21:13:58 +0800 Subject: [PATCH 06/15] Address comments --- .../datastrato/gravitino/GravitinoEnv.java | 11 ------- .../catalog/EntityCombinedSchema.java | 8 +++-- .../catalog/EntityCombinedTable.java | 8 +++-- .../catalog/EntityCombinedTopic.java | 8 +++-- .../catalog/OperationDispatcher.java | 10 +++++++ .../catalog/SchemaOperationDispatcher.java | 25 ++++++++-------- .../catalog/TableOperationDispatcher.java | 29 ++++++++++--------- .../catalog/TopicOperationDispatcher.java | 20 +++++-------- .../TestSchemaNormalizeDispatcher.java | 2 +- .../TestSchemaOperationDispatcher.java | 5 ++-- .../web/rest/TestCatalogOperations.java | 5 ++-- .../web/rest/TestFilesetOperations.java | 5 ++-- .../web/rest/TestMetalakeOperations.java | 5 ++-- .../web/rest/TestPartitionOperations.java | 5 ++-- .../server/web/rest/TestSchemaOperations.java | 5 ++-- .../server/web/rest/TestTableOperations.java | 5 ++-- .../server/web/rest/TestTopicOperations.java | 5 ++-- 17 files changed, 86 insertions(+), 75 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java index 3a98b48fc27..a2c9b710a1e 100644 --- a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java +++ b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java @@ -110,17 +110,6 @@ public void setLockManager(LockManager lockManager) { this.lockManager = lockManager; } - /** - * This method is used for testing purposes only to set the access manager for test in package - * `com.datastrato.gravitino.server.web.rest` and `com.datastrato.gravitino.authorization`. - * - * @param accessControlManager The access control manager to be set. - */ - @VisibleForTesting - public void setAccessControlManager(AccessControlManager accessControlManager) { - this.accessControlManager = accessControlManager; - } - /** * Initialize the Gravitino environment. * diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java index f64ab40cc31..1a8c8805b21 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java @@ -6,9 +6,9 @@ import com.datastrato.gravitino.Audit; import com.datastrato.gravitino.Schema; +import com.datastrato.gravitino.StringIdentifier; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.SchemaEntity; -import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -24,6 +24,8 @@ public final class EntityCombinedSchema implements Schema { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. + // Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedSchema(Schema schema, SchemaEntity schemaEntity) { @@ -85,7 +87,7 @@ public boolean imported() { return imported; } - Map schemaProperties() { - return Collections.unmodifiableMap(schema.properties()); + StringIdentifier stringIdentifier() { + return StringIdentifier.fromProperties(schema.properties()); } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java index d0e5efacfc8..7b1475ac85a 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java @@ -5,6 +5,7 @@ package com.datastrato.gravitino.catalog; import com.datastrato.gravitino.Audit; +import com.datastrato.gravitino.StringIdentifier; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.TableEntity; import com.datastrato.gravitino.rel.Column; @@ -14,7 +15,6 @@ import com.datastrato.gravitino.rel.expressions.sorts.SortOrder; import com.datastrato.gravitino.rel.expressions.transforms.Transform; import com.datastrato.gravitino.rel.indexes.Index; -import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -31,6 +31,8 @@ public final class EntityCombinedTable implements Table { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. + // Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedTable(Table table, TableEntity tableEntity) { @@ -122,7 +124,7 @@ public Audit auditInfo() { : mergedAudit.merge(tableEntity.auditInfo(), true /* overwrite */); } - Map tableProperties() { - return Collections.unmodifiableMap(table.properties()); + StringIdentifier stringIdentifier() { + return StringIdentifier.fromProperties(table.properties()); } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java index 19bef7f63fc..e543a6e6597 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java @@ -5,10 +5,10 @@ package com.datastrato.gravitino.catalog; import com.datastrato.gravitino.Audit; +import com.datastrato.gravitino.StringIdentifier; import com.datastrato.gravitino.messaging.Topic; import com.datastrato.gravitino.meta.AuditInfo; import com.datastrato.gravitino.meta.TopicEntity; -import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -24,6 +24,8 @@ public class EntityCombinedTopic implements Topic { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. + // Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedTopic(Topic topic, TopicEntity topicEntity) { @@ -85,7 +87,7 @@ public boolean imported() { return imported; } - Map topicProperties() { - return Collections.unmodifiableMap(topic.properties()); + StringIdentifier stringIdentifier() { + return StringIdentifier.fromProperties(topic.properties()); } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java index a24a258d398..18efa71ee8a 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java @@ -6,6 +6,7 @@ import static com.datastrato.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForAlter; +import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.EntityStore; import com.datastrato.gravitino.HasIdentifier; import com.datastrato.gravitino.NameIdentifier; @@ -272,6 +273,15 @@ boolean isManagedEntity(NameIdentifier catalogIdent, Capability.Scope scope) { IllegalArgumentException.class); } + boolean isEntityExist(NameIdentifier ident, Entity.EntityType type) { + try { + return store.exists(ident, type); + } catch (Exception e) { + LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); + throw new RuntimeException("Fail to access underlying storage", e); + } + } + static final class FormattedErrorMessages { static final String STORE_OP_FAILURE = "Failed to {} entity for {} in " diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index cbf5e279975..f31437cf1d9 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -310,14 +310,20 @@ private boolean importSchema(NameIdentifier identifier) { return false; } - StringIdentifier stringId = getStringIdFromProperties(combinedSchema.schemaProperties()); + StringIdentifier stringId = null; + try { + stringId = combinedSchema.stringIdentifier(); + } catch (IllegalArgumentException ie) { + LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); + } + long uid; if (stringId != null) { - // If the entity in the store doesn't match the external system, we use the data + // If the entity in the store doesn't match the one in the external system, we use the data // of external system to correct it. uid = stringId.id(); } else { - // If store doesn't exist entity, we sync the entity from the external system. + // If entity doesn't exist, we import the entity from the external system. uid = idGenerator.nextId(); } @@ -361,6 +367,8 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { catalogIdentifier, HasPropertyMetadata::schemaPropertiesMetadata, schema.properties())) + // The meta of managed schema is stored by Gravitino, + // We don't need to import it again. .withImported(true); } @@ -373,7 +381,7 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { catalogIdentifier, HasPropertyMetadata::schemaPropertiesMetadata, schema.properties())) - .withImported(isEntityExist(ident)); + .withImported(isEntityExist(ident, SCHEMA)); } SchemaEntity schemaEntity = @@ -393,13 +401,4 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { schema.properties())) .withImported(imported); } - - private boolean isEntityExist(NameIdentifier ident) { - try { - return store.exists(ident, SCHEMA); - } catch (Exception e) { - LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); - throw new RuntimeException("Fail to access underlying storage"); - } - } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 836c3aa7237..a9bdb37fda1 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -374,14 +374,20 @@ private boolean importTable(NameIdentifier identifier) { return false; } - StringIdentifier stringId = getStringIdFromProperties(combinedTable.tableProperties()); + StringIdentifier stringId = null; + try { + stringId = combinedTable.stringIdentifier(); + } catch (IllegalArgumentException ie) { + LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); + } + long uid; if (stringId != null) { // If the entity in the store doesn't match the external system, we use the data // of external system to correct it. uid = stringId.id(); } else { - // If store doesn't exist entity, we sync the entity from the external system. + // If entity doesn't exist, we import the entity from the external system. uid = idGenerator.nextId(); } @@ -407,15 +413,6 @@ private boolean importTable(NameIdentifier identifier) { return true; } - private boolean isEntityExist(NameIdentifier ident) { - try { - return store.exists(ident, TABLE); - } catch (Exception e) { - LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); - throw new RuntimeException("Fail to access underlying storage"); - } - } - private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); Table table = @@ -425,7 +422,8 @@ private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { NoSuchTableException.class); StringIdentifier stringId = getStringIdFromProperties(table.properties()); - // Case 1: The table is not created by Gravitino. + // Case 1: The table is not created by Gravitino or the backend storage does not support storing + // string identifier. if (stringId == null) { return EntityCombinedTable.of(table) .withHiddenPropertiesSet( @@ -433,7 +431,10 @@ private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { catalogIdentifier, HasPropertyMetadata::tablePropertiesMetadata, table.properties())) - .withImported(isEntityExist(ident)); + // Some tables don't have properties or are not created by Gravitino, + // we can't use stringIdentifier to judge whether schema is ever imported or not. + // We need to check whether the entity exists. + .withImported(isEntityExist(ident, TABLE)); } TableEntity tableEntity = @@ -443,6 +444,8 @@ private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { "GET", stringId.id()); + // If the entity is inconsistent from the one of the external system, + // we should import it. boolean imported = tableEntity != null; return EntityCombinedTable.of(table, tableEntity) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index 1bc69760633..ac6e0f3374b 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -278,7 +278,12 @@ private boolean importTopic(NameIdentifier identifier) { return false; } - StringIdentifier stringId = getStringIdFromProperties(combinedTopic.topicProperties()); + StringIdentifier stringId = null; + try { + stringId = combinedTopic.stringIdentifier(); + } catch (IllegalArgumentException ie) { + LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); + } long uid; if (stringId != null) { @@ -286,7 +291,7 @@ private boolean importTopic(NameIdentifier identifier) { // of external system to correct it. uid = stringId.id(); } else { - // If store doesn't exist entity, we sync the entity from the external system. + // If entity doesn't exist, we import the entity from the external system. uid = idGenerator.nextId(); } @@ -315,15 +320,6 @@ private boolean importTopic(NameIdentifier identifier) { return true; } - private boolean isEntityExist(NameIdentifier ident) { - try { - return store.exists(ident, TOPIC); - } catch (Exception e) { - LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); - throw new RuntimeException("Fail to access underlying storage"); - } - } - private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { NameIdentifier catalogIdent = getCatalogIdentifier(ident); Topic topic = @@ -342,7 +338,7 @@ private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())) - .withImported(isEntityExist(ident)); + .withImported(isEntityExist(ident, TOPIC)); } TopicEntity topicEntity = diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaNormalizeDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaNormalizeDispatcher.java index 44a8c21f3ad..c696e872d8c 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaNormalizeDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaNormalizeDispatcher.java @@ -21,7 +21,7 @@ public class TestSchemaNormalizeDispatcher extends TestOperationDispatcher { private static SchemaNormalizeDispatcher schemaNormalizeDispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { TestSchemaOperationDispatcher.initialize(); schemaNormalizeDispatcher = new SchemaNormalizeDispatcher(TestSchemaOperationDispatcher.dispatcher); diff --git a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java index 1cf998e565c..0517c6195f3 100644 --- a/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java +++ b/core/src/test/java/com/datastrato/gravitino/catalog/TestSchemaOperationDispatcher.java @@ -34,6 +34,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -43,14 +44,14 @@ public class TestSchemaOperationDispatcher extends TestOperationDispatcher { static SchemaOperationDispatcher dispatcher; @BeforeAll - public static void initialize() throws IOException { + public static void initialize() throws IOException, IllegalAccessException { dispatcher = new SchemaOperationDispatcher(catalogManager, entityStore, idGenerator); Config config = mock(Config.class); doReturn(100000L).when(config).get(Configs.TREE_LOCK_MAX_NODE_IN_MEMORY); doReturn(1000L).when(config).get(Configs.TREE_LOCK_MIN_NODE_IN_MEMORY); doReturn(36000L).when(config).get(Configs.TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Test diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestCatalogOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestCatalogOperations.java index 2ffb138a452..e294b05f093 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestCatalogOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestCatalogOperations.java @@ -46,6 +46,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -69,12 +70,12 @@ public HttpServletRequest get() { private CatalogManager manager = mock(CatalogManager.class); @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestFilesetOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestFilesetOperations.java index 96ebd196c2c..c1f342de58c 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestFilesetOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestFilesetOperations.java @@ -44,6 +44,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -72,12 +73,12 @@ public HttpServletRequest get() { private final String schema = "schema1"; @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeOperations.java index a00afd62166..a4fe3eca64c 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeOperations.java @@ -42,6 +42,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -65,12 +66,12 @@ public HttpServletRequest get() { private MetalakeManager metalakeManager = mock(MetalakeManager.class); @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPartitionOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPartitionOperations.java index 9de31fb91f5..00d3027dc83 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPartitionOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestPartitionOperations.java @@ -40,6 +40,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -84,12 +85,12 @@ public HttpServletRequest get() { private final String table = "table1"; @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java index a711cc9351e..b4f119c87dd 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestSchemaOperations.java @@ -44,6 +44,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -71,12 +72,12 @@ public HttpServletRequest get() { private final String catalog = "catalog1"; @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTableOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTableOperations.java index 40851410206..3766f69cee0 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTableOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTableOperations.java @@ -71,6 +71,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -100,12 +101,12 @@ public HttpServletRequest get() { private final String schema = "schema1"; @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override diff --git a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTopicOperations.java b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTopicOperations.java index a65b75fdf4a..21e740ba928 100644 --- a/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTopicOperations.java +++ b/server/src/test/java/com/datastrato/gravitino/server/web/rest/TestTopicOperations.java @@ -43,6 +43,7 @@ import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.reflect.FieldUtils; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -69,12 +70,12 @@ public HttpServletRequest get() { private final String schema = "default"; @BeforeAll - public static void setup() { + public static void setup() throws IllegalAccessException { Config config = mock(Config.class); Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); - GravitinoEnv.getInstance().setLockManager(new LockManager(config)); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); } @Override From f39720a2314e31cd06e40b92eb3fa1a5fb22c1ab Mon Sep 17 00:00:00 2001 From: Rory Date: Tue, 11 Jun 2024 21:15:38 +0800 Subject: [PATCH 07/15] Remove unused code --- .../java/com/datastrato/gravitino/GravitinoEnv.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java index a2c9b710a1e..326e75f721a 100644 --- a/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java +++ b/core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java @@ -40,7 +40,6 @@ import com.datastrato.gravitino.metrics.source.JVMMetricsSource; import com.datastrato.gravitino.storage.IdGenerator; import com.datastrato.gravitino.storage.RandomIdGenerator; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -98,18 +97,6 @@ public static GravitinoEnv getInstance() { return InstanceHolder.INSTANCE; } - /** - * This method is used for testing purposes only to set the lock manager for test in package as - * tree lock depends on the lock manager and we did not mock the lock manager in the test, so we - * need to set the lock manager for test. - * - * @param lockManager The lock manager to be set. - */ - @VisibleForTesting - public void setLockManager(LockManager lockManager) { - this.lockManager = lockManager; - } - /** * Initialize the Gravitino environment. * From 08cf4e2410d72cd24be8d6daf888663a5172aa84 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 12 Jun 2024 10:26:24 +0800 Subject: [PATCH 08/15] Address comments --- .../datastrato/gravitino/catalog/EntityCombinedSchema.java | 1 + .../datastrato/gravitino/catalog/EntityCombinedTable.java | 1 + .../datastrato/gravitino/catalog/EntityCombinedTopic.java | 1 + .../gravitino/catalog/SchemaOperationDispatcher.java | 5 +++-- .../gravitino/catalog/TableOperationDispatcher.java | 2 +- .../gravitino/catalog/TopicOperationDispatcher.java | 2 +- 6 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java index 1a8c8805b21..6c0e064958b 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java @@ -24,6 +24,7 @@ public final class EntityCombinedSchema implements Schema { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. // Otherwise, we should import the external entity to the storage backend. private boolean imported; diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java index 7b1475ac85a..6faacb7f610 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java @@ -31,6 +31,7 @@ public final class EntityCombinedTable implements Table { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. // Otherwise, we should import the external entity to the storage backend. private boolean imported; diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java index e543a6e6597..60b557bd51f 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java @@ -24,6 +24,7 @@ public class EntityCombinedTopic implements Topic { // Sets of properties that should be hidden from the user. private Set hiddenProperties; + // If imported is true, it means that storage backend have stored the correct entity. // Otherwise, we should import the external entity to the storage backend. private boolean imported; diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index f31437cf1d9..bc7c103f2ec 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -164,7 +164,7 @@ public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException { EntityCombinedSchema schema = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedSchema(ident)); - if (schema == null || schema.imported()) { + if (schema.imported()) { return schema; } @@ -373,7 +373,8 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { } StringIdentifier stringId = getStringIdFromProperties(schema.properties()); - // Case 1: The schema is not created by Gravitino. + // Case 1: The schema is not created by Gravitino or the backend storage does not support + // storing string identifiers. if (stringId == null) { return EntityCombinedSchema.of(schema) .withHiddenPropertiesSet( diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index a9bdb37fda1..827bbbf30dc 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -85,7 +85,7 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { EntityCombinedTable table = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTable(ident)); - if (table == null || table.imported()) { + if (table.imported()) { return table; } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index ac6e0f3374b..a9c9e288a36 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -76,7 +76,7 @@ public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { EntityCombinedTopic topic = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTopic(ident)); - if (topic == null || topic.imported()) { + if (topic.imported()) { return topic; } From e762b5888d1451ebdbc76f51b32a4c0511180aa5 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 12 Jun 2024 10:34:56 +0800 Subject: [PATCH 09/15] Add comment --- .../gravitino/catalog/SchemaOperationDispatcher.java | 10 ++++++++-- .../gravitino/catalog/TableOperationDispatcher.java | 10 ++++++++-- .../gravitino/catalog/TopicOperationDispatcher.java | 10 ++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index bc7c103f2ec..fcf358e0d3d 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -168,8 +168,14 @@ public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException { return schema; } - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importSchema(ident)); + boolean imported = + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), + LockType.WRITE, + () -> importSchema(ident)); + if (imported) { + LOG.info("Gravitino imports the schema {} successfully", ident); + } return schema; } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 827bbbf30dc..539ccc25f51 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -92,8 +92,14 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { if (GravitinoEnv.getInstance() .schemaDispatcher() .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTable(ident)); + boolean imported = + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), + LockType.WRITE, + () -> importTable(ident)); + if (imported) { + LOG.info("Gravitino imports the table {} successfully", ident); + } } return table; diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index a9c9e288a36..c659b44e3f1 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -83,8 +83,14 @@ public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { if (GravitinoEnv.getInstance() .schemaDispatcher() .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTopic(ident)); + boolean imported = + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), + LockType.WRITE, + () -> importTopic(ident)); + if (imported) { + LOG.info("Gravitino imports the topic {} successfully", ident); + } } return topic; } From ad5400aca8f09a87e1a0e72cba2bbefcdb54c0c9 Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 13 Jun 2024 10:34:19 +0800 Subject: [PATCH 10/15] Address comments --- .../gravitino/catalog/OperationDispatcher.java | 2 +- .../catalog/SchemaOperationDispatcher.java | 13 +++++-------- .../gravitino/catalog/TableOperationDispatcher.java | 12 +++++------- .../gravitino/catalog/TopicOperationDispatcher.java | 12 +++++------- 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java index 18efa71ee8a..d7459f8f3dc 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java @@ -273,7 +273,7 @@ boolean isManagedEntity(NameIdentifier catalogIdent, Capability.Scope scope) { IllegalArgumentException.class); } - boolean isEntityExist(NameIdentifier ident, Entity.EntityType type) { + protected boolean isEntityExist(NameIdentifier ident, Entity.EntityType type) { try { return store.exists(ident, type); } catch (Exception e) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index fcf358e0d3d..e81861f1076 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -168,16 +168,13 @@ public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException { return schema; } - boolean imported = + EntityCombinedSchema importedSchema = TreeLockUtils.doWithTreeLock( NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importSchema(ident)); - if (imported) { - LOG.info("Gravitino imports the schema {} successfully", ident); - } - return schema; + return importedSchema; } /** @@ -310,10 +307,10 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty : droppedFromCatalog; } - private boolean importSchema(NameIdentifier identifier) { + private EntityCombinedSchema importSchema(NameIdentifier identifier) { EntityCombinedSchema combinedSchema = loadCombinedSchema(identifier); if (combinedSchema.imported()) { - return false; + return combinedSchema; } StringIdentifier stringId = null; @@ -353,7 +350,7 @@ private boolean importSchema(NameIdentifier identifier) { throw new RuntimeException("Fail to access underlying storage"); } - return true; + return combinedSchema; } private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 539ccc25f51..5cf4862c04e 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -92,14 +92,12 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { if (GravitinoEnv.getInstance() .schemaDispatcher() .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - boolean imported = + EntityCombinedTable importedTable = TreeLockUtils.doWithTreeLock( NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTable(ident)); - if (imported) { - LOG.info("Gravitino imports the table {} successfully", ident); - } + return importedTable; } return table; @@ -373,11 +371,11 @@ public boolean purgeTable(NameIdentifier ident) throws UnsupportedOperationExcep : droppedFromCatalog; } - private boolean importTable(NameIdentifier identifier) { + private EntityCombinedTable importTable(NameIdentifier identifier) { EntityCombinedTable combinedTable = loadCombinedTable(identifier); if (combinedTable.imported()) { - return false; + return combinedTable; } StringIdentifier stringId = null; @@ -416,7 +414,7 @@ private boolean importTable(NameIdentifier identifier) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); throw new RuntimeException("Fail to access underlying storage"); } - return true; + return combinedTable; } private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index c659b44e3f1..d6cada233f4 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -83,14 +83,12 @@ public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { if (GravitinoEnv.getInstance() .schemaDispatcher() .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - boolean imported = + EntityCombinedTopic combinedTopic = TreeLockUtils.doWithTreeLock( NameIdentifier.of(ident.namespace().levels()), LockType.WRITE, () -> importTopic(ident)); - if (imported) { - LOG.info("Gravitino imports the topic {} successfully", ident); - } + return combinedTopic; } return topic; } @@ -276,12 +274,12 @@ public boolean dropTopic(NameIdentifier ident) { : droppedFromCatalog; } - private boolean importTopic(NameIdentifier identifier) { + private EntityCombinedTopic importTopic(NameIdentifier identifier) { EntityCombinedTopic combinedTopic = loadCombinedTopic(identifier); if (combinedTopic.imported()) { - return false; + return combinedTopic; } StringIdentifier stringId = null; @@ -323,7 +321,7 @@ private boolean importTopic(NameIdentifier identifier) { throw new RuntimeException("Fail to access underlying storage"); } - return true; + return combinedTopic; } private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { From e12c766282070d867ec77560e494b2ebd4de80a5 Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 13 Jun 2024 19:45:30 +0800 Subject: [PATCH 11/15] Address comments --- .../datastrato/gravitino/catalog/SchemaOperationDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index e81861f1076..49078a4eb27 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -376,7 +376,7 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { } StringIdentifier stringId = getStringIdFromProperties(schema.properties()); - // Case 1: The schema is not created by Gravitino or the backend storage does not support + // Case 1: The schema is not created by Gravitino or the external system does not support // storing string identifiers. if (stringId == null) { return EntityCombinedSchema.of(schema) From bde6b0d3d8b63bd5e1db5bcb2b901bb8b3c470fa Mon Sep 17 00:00:00 2001 From: Jerry Shao Date: Mon, 17 Jun 2024 20:34:10 +0800 Subject: [PATCH 12/15] Slightly refactor the code --- .../catalog/EntityCombinedSchema.java | 7 +- .../catalog/EntityCombinedTable.java | 6 +- .../catalog/EntityCombinedTopic.java | 6 +- .../catalog/OperationDispatcher.java | 90 +++++++++---------- .../catalog/SchemaOperationDispatcher.java | 62 +++++++------ .../catalog/TableOperationDispatcher.java | 65 +++++++------- .../catalog/TopicOperationDispatcher.java | 77 ++++++++-------- 7 files changed, 164 insertions(+), 149 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java index 6c0e064958b..aa7d575b82f 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java @@ -20,18 +20,21 @@ public final class EntityCombinedSchema implements Schema { private final Schema schema; + private final SchemaEntity schemaEntity; // Sets of properties that should be hidden from the user. private Set hiddenProperties; - // If imported is true, it means that storage backend have stored the correct entity. - // Otherwise, we should import the external entity to the storage backend. + // Field "imported" is used to indicate whether the entity has been imported to Gravitino + // managed storage backend. If "imported" is true, it means that storage backend have stored + // the correct entity. Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedSchema(Schema schema, SchemaEntity schemaEntity) { this.schema = schema; this.schemaEntity = schemaEntity; + this.imported = false; } public static EntityCombinedSchema of(Schema schema, SchemaEntity schemaEntity) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java index 6faacb7f610..6478a260145 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java @@ -32,13 +32,15 @@ public final class EntityCombinedTable implements Table { // Sets of properties that should be hidden from the user. private Set hiddenProperties; - // If imported is true, it means that storage backend have stored the correct entity. - // Otherwise, we should import the external entity to the storage backend. + // Field "imported" is used to indicate whether the entity has been imported to Gravitino + // managed storage backend. If "imported" is true, it means that storage backend have stored + // the correct entity. Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedTable(Table table, TableEntity tableEntity) { this.table = table; this.tableEntity = tableEntity; + this.imported = false; } public static EntityCombinedTable of(Table table, TableEntity tableEntity) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java index 60b557bd51f..3ddb4a20885 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java @@ -25,13 +25,15 @@ public class EntityCombinedTopic implements Topic { // Sets of properties that should be hidden from the user. private Set hiddenProperties; - // If imported is true, it means that storage backend have stored the correct entity. - // Otherwise, we should import the external entity to the storage backend. + // Field "imported" is used to indicate whether the entity has been imported to Gravitino + // managed storage backend. If "imported" is true, it means that storage backend have stored + // the correct entity. Otherwise, we should import the external entity to the storage backend. private boolean imported; private EntityCombinedTopic(Topic topic, TopicEntity topicEntity) { this.topic = topic; this.topicEntity = topicEntity; + this.imported = false; } public static EntityCombinedTopic of(Topic topic, TopicEntity topicEntity) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java index d7459f8f3dc..4272b5c24f9 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java @@ -45,7 +45,7 @@ public abstract class OperationDispatcher { protected final EntityStore store; - final IdGenerator idGenerator; + protected final IdGenerator idGenerator; /** * Creates a new CatalogOperationDispatcher instance. @@ -61,7 +61,21 @@ public OperationDispatcher( this.idGenerator = idGenerator; } - R doWithTable( + protected Capability getCatalogCapability(NameIdentifier ident) { + return doWithCatalog( + getCatalogIdentifier(ident), + CatalogManager.CatalogWrapper::capabilities, + IllegalArgumentException.class); + } + + protected Capability getCatalogCapability(Namespace namespace) { + return doWithCatalog( + getCatalogIdentifier(NameIdentifier.of(namespace.levels())), + CatalogManager.CatalogWrapper::capabilities, + IllegalArgumentException.class); + } + + protected R doWithTable( NameIdentifier tableIdent, ThrowableFunction fn, Class ex) throws E { try { @@ -79,7 +93,7 @@ R doWithTable( } } - R doWithCatalog( + protected R doWithCatalog( NameIdentifier ident, ThrowableFunction fn, Class ex) throws E { try { @@ -96,7 +110,7 @@ R doWithCatalog( } } - R doWithCatalog( + protected R doWithCatalog( NameIdentifier ident, ThrowableFunction fn, Class ex1, @@ -119,21 +133,7 @@ R doWithCatalog( } } - Capability getCatalogCapability(NameIdentifier ident) { - return doWithCatalog( - getCatalogIdentifier(ident), - CatalogManager.CatalogWrapper::capabilities, - IllegalArgumentException.class); - } - - Capability getCatalogCapability(Namespace namespace) { - return doWithCatalog( - getCatalogIdentifier(NameIdentifier.of(namespace.levels())), - CatalogManager.CatalogWrapper::capabilities, - IllegalArgumentException.class); - } - - Set getHiddenPropertyNames( + protected Set getHiddenPropertyNames( NameIdentifier catalogIdent, ThrowableFunction provider, Map properties) { @@ -150,7 +150,7 @@ Set getHiddenPropertyNames( IllegalArgumentException.class); } - void validateAlterProperties( + protected void validateAlterProperties( NameIdentifier ident, ThrowableFunction provider, T... changes) { @@ -167,27 +167,6 @@ void validateAlterProperties( IllegalArgumentException.class); } - private Map getPropertiesForSet(T... t) { - Map properties = Maps.newHashMap(); - for (T item : t) { - if (item instanceof TableChange.SetProperty) { - TableChange.SetProperty setProperty = (TableChange.SetProperty) item; - properties.put(setProperty.getProperty(), setProperty.getValue()); - } else if (item instanceof SchemaChange.SetProperty) { - SchemaChange.SetProperty setProperty = (SchemaChange.SetProperty) item; - properties.put(setProperty.getProperty(), setProperty.getValue()); - } else if (item instanceof FilesetChange.SetProperty) { - FilesetChange.SetProperty setProperty = (FilesetChange.SetProperty) item; - properties.put(setProperty.getProperty(), setProperty.getValue()); - } else if (item instanceof TopicChange.SetProperty) { - TopicChange.SetProperty setProperty = (TopicChange.SetProperty) item; - properties.put(setProperty.getProperty(), setProperty.getValue()); - } - } - - return properties; - } - private Map getPropertiesForDelete(T... t) { Map properties = Maps.newHashMap(); for (T item : t) { @@ -209,7 +188,7 @@ private Map getPropertiesForDelete(T... t) { return properties; } - StringIdentifier getStringIdFromProperties(Map properties) { + protected StringIdentifier getStringIdFromProperties(Map properties) { try { StringIdentifier stringId = StringIdentifier.fromProperties(properties); if (stringId == null) { @@ -222,7 +201,7 @@ StringIdentifier getStringIdFromProperties(Map properties) { } } - R operateOnEntity( + protected R operateOnEntity( NameIdentifier ident, ThrowableFunction fn, String opName, long id) { R ret = null; try { @@ -248,7 +227,7 @@ R operateOnEntity( // TODO(xun): Remove this method when we implement a better way to get the catalog identifier // [#257] Add an explicit get catalog functions in NameIdentifier - NameIdentifier getCatalogIdentifier(NameIdentifier ident) { + protected NameIdentifier getCatalogIdentifier(NameIdentifier ident) { NameIdentifier.check( ident.name() != null, "The name variable in the NameIdentifier must have value."); Namespace.check( @@ -278,10 +257,31 @@ protected boolean isEntityExist(NameIdentifier ident, Entity.EntityType type) { return store.exists(ident, type); } catch (Exception e) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "exists", ident, e); - throw new RuntimeException("Fail to access underlying storage", e); + throw new RuntimeException("Fail to check if entity is existed", e); } } + private Map getPropertiesForSet(T... t) { + Map properties = Maps.newHashMap(); + for (T item : t) { + if (item instanceof TableChange.SetProperty) { + TableChange.SetProperty setProperty = (TableChange.SetProperty) item; + properties.put(setProperty.getProperty(), setProperty.getValue()); + } else if (item instanceof SchemaChange.SetProperty) { + SchemaChange.SetProperty setProperty = (SchemaChange.SetProperty) item; + properties.put(setProperty.getProperty(), setProperty.getValue()); + } else if (item instanceof FilesetChange.SetProperty) { + FilesetChange.SetProperty setProperty = (FilesetChange.SetProperty) item; + properties.put(setProperty.getProperty(), setProperty.getValue()); + } else if (item instanceof TopicChange.SetProperty) { + TopicChange.SetProperty setProperty = (TopicChange.SetProperty) item; + properties.put(setProperty.getProperty(), setProperty.getValue()); + } + } + + return properties; + } + static final class FormattedErrorMessages { static final String STORE_OP_FAILURE = "Failed to {} entity for {} in " diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index 49078a4eb27..273a7d0617a 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -161,20 +161,25 @@ public Schema createSchema(NameIdentifier ident, String comment, Map loadCombinedSchema(ident)); + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadSchema(ident)); if (schema.imported()) { return schema; } - EntityCombinedSchema importedSchema = - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), - LockType.WRITE, - () -> importSchema(ident)); + if (!schema.imported()) { + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(ident.namespace().levels()), + LockType.WRITE, + () -> { + importSchema(ident); + return null; + }); + } - return importedSchema; + return schema; } /** @@ -307,15 +312,15 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty : droppedFromCatalog; } - private EntityCombinedSchema importSchema(NameIdentifier identifier) { - EntityCombinedSchema combinedSchema = loadCombinedSchema(identifier); - if (combinedSchema.imported()) { - return combinedSchema; + private void importSchema(NameIdentifier identifier) { + EntityCombinedSchema schema = internalLoadSchema(identifier); + if (schema.imported()) { + return; } StringIdentifier stringId = null; try { - stringId = combinedSchema.stringIdentifier(); + stringId = schema.stringIdentifier(); } catch (IllegalArgumentException ie) { LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); } @@ -324,6 +329,11 @@ private EntityCombinedSchema importSchema(NameIdentifier identifier) { if (stringId != null) { // If the entity in the store doesn't match the one in the external system, we use the data // of external system to correct it. + LOG.warn( + "The Schema uid {} existed but still needs to be imported, this could be happened " + + "when Schema is renamed by external systems not controlled by Gravitino. In this case, " + + "we need to overwrite the stored entity to keep consistency.", + stringId); uid = stringId.id(); } else { // If entity doesn't exist, we import the entity from the external system. @@ -337,23 +347,21 @@ private EntityCombinedSchema importSchema(NameIdentifier identifier) { .withNamespace(identifier.namespace()) .withAuditInfo( AuditInfo.builder() - .withCreator(combinedSchema.auditInfo().creator()) - .withCreateTime(combinedSchema.auditInfo().createTime()) - .withLastModifier(combinedSchema.auditInfo().lastModifier()) - .withLastModifiedTime(combinedSchema.auditInfo().lastModifiedTime()) + .withCreator(schema.auditInfo().creator()) + .withCreateTime(schema.auditInfo().createTime()) + .withLastModifier(schema.auditInfo().lastModifier()) + .withLastModifiedTime(schema.auditInfo().lastModifiedTime()) .build()) .build(); try { store.put(schemaEntity, true); } catch (Exception e) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); - throw new RuntimeException("Fail to access underlying storage"); + throw new RuntimeException("Fail to import schema entity to the store.", e); } - - return combinedSchema; } - private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { + private EntityCombinedSchema internalLoadSchema(NameIdentifier ident) { NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); Schema schema = doWithCatalog( @@ -361,7 +369,7 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { c -> c.doWithSchemaOps(s -> s.loadSchema(ident)), NoSuchSchemaException.class); - // If the Schema is maintained by the Gravitino's store, we don't have to load again. + // If the Schema is maintained by the entity store, we don't have to import. boolean isManagedSchema = isManagedEntity(catalogIdentifier, Capability.Scope.SCHEMA); if (isManagedSchema) { return EntityCombinedSchema.of(schema) @@ -370,9 +378,8 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { catalogIdentifier, HasPropertyMetadata::schemaPropertiesMetadata, schema.properties())) - // The meta of managed schema is stored by Gravitino, - // We don't need to import it again. - .withImported(true); + // The meta of managed schema is stored by Gravitino, we don't need to import it. + .withImported(true /* imported */); } StringIdentifier stringId = getStringIdFromProperties(schema.properties()); @@ -385,6 +392,9 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { catalogIdentifier, HasPropertyMetadata::schemaPropertiesMetadata, schema.properties())) + // For some catalogs like PG, the identifier information is not stored in the schema's + // metadata, we need to check if this schema is existed in the store, if so we don't + // need to import. .withImported(isEntityExist(ident, SCHEMA)); } @@ -395,14 +405,12 @@ private EntityCombinedSchema loadCombinedSchema(NameIdentifier ident) { "GET", stringId.id()); - boolean imported = schemaEntity != null; - return EntityCombinedSchema.of(schema, schemaEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdentifier, HasPropertyMetadata::schemaPropertiesMetadata, schema.properties())) - .withImported(imported); + .withImported(schemaEntity != null); } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 5cf4862c04e..169b002e9a7 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -83,21 +83,22 @@ public NameIdentifier[] listTables(Namespace namespace) throws NoSuchSchemaExcep @Override public Table loadTable(NameIdentifier ident) throws NoSuchTableException { EntityCombinedTable table = - TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTable(ident)); - - if (table.imported()) { - return table; - } - - if (GravitinoEnv.getInstance() - .schemaDispatcher() - .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - EntityCombinedTable importedTable = - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), - LockType.WRITE, - () -> importTable(ident)); - return importedTable; + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadTable(ident)); + + if (!table.imported()) { + // Load the schema to make sure the schema is imported. + SchemaDispatcher schemaDispatcher = GravitinoEnv.getInstance().schemaDispatcher(); + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + schemaDispatcher.loadSchema(schemaIdent); + + // Import the table. + TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + importTable(ident); + return null; + }); } return table; @@ -371,16 +372,16 @@ public boolean purgeTable(NameIdentifier ident) throws UnsupportedOperationExcep : droppedFromCatalog; } - private EntityCombinedTable importTable(NameIdentifier identifier) { - EntityCombinedTable combinedTable = loadCombinedTable(identifier); + private void importTable(NameIdentifier identifier) { + EntityCombinedTable table = internalLoadTable(identifier); - if (combinedTable.imported()) { - return combinedTable; + if (table.imported()) { + return; } StringIdentifier stringId = null; try { - stringId = combinedTable.stringIdentifier(); + stringId = table.stringIdentifier(); } catch (IllegalArgumentException ie) { LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); } @@ -389,6 +390,11 @@ private EntityCombinedTable importTable(NameIdentifier identifier) { if (stringId != null) { // If the entity in the store doesn't match the external system, we use the data // of external system to correct it. + LOG.warn( + "The Table uid {} existed but still need to be imported, this could be happened " + + "when Table is renamed by external systems not controlled by Gravitino. In this case, " + + "we need to overwrite the stored entity to keep the consistency.", + stringId); uid = stringId.id(); } else { // If entity doesn't exist, we import the entity from the external system. @@ -402,22 +408,21 @@ private EntityCombinedTable importTable(NameIdentifier identifier) { .withNamespace(identifier.namespace()) .withAuditInfo( AuditInfo.builder() - .withCreator(combinedTable.auditInfo().creator()) - .withCreateTime(combinedTable.auditInfo().createTime()) - .withLastModifier(combinedTable.auditInfo().lastModifier()) - .withLastModifiedTime(combinedTable.auditInfo().lastModifiedTime()) + .withCreator(table.auditInfo().creator()) + .withCreateTime(table.auditInfo().createTime()) + .withLastModifier(table.auditInfo().lastModifier()) + .withLastModifiedTime(table.auditInfo().lastModifiedTime()) .build()) .build(); try { store.put(tableEntity, true); } catch (Exception e) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); - throw new RuntimeException("Fail to access underlying storage"); + throw new RuntimeException("Fail to import the table entity to the store.", e); } - return combinedTable; } - private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { + private EntityCombinedTable internalLoadTable(NameIdentifier ident) { NameIdentifier catalogIdentifier = getCatalogIdentifier(ident); Table table = doWithCatalog( @@ -448,16 +453,12 @@ private EntityCombinedTable loadCombinedTable(NameIdentifier ident) { "GET", stringId.id()); - // If the entity is inconsistent from the one of the external system, - // we should import it. - boolean imported = tableEntity != null; - return EntityCombinedTable.of(table, tableEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdentifier, HasPropertyMetadata::tablePropertiesMetadata, table.properties())) - .withImported(imported); + .withImported(tableEntity != null); } } diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java index d6cada233f4..d8d31425f23 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TopicOperationDispatcher.java @@ -74,22 +74,25 @@ public NameIdentifier[] listTopics(Namespace namespace) throws NoSuchSchemaExcep @Override public Topic loadTopic(NameIdentifier ident) throws NoSuchTopicException { EntityCombinedTopic topic = - TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> loadCombinedTopic(ident)); - - if (topic.imported()) { - return topic; + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadTopic(ident)); + + if (!topic.imported()) { + // Load the schema to make sure the schema is imported. + // This is not necessary for Kafka catalog. + SchemaDispatcher schemaDispatcher = GravitinoEnv.getInstance().schemaDispatcher(); + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + schemaDispatcher.loadSchema(schemaIdent); + + // Import the topic + TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + importTopic(ident); + return null; + }); } - if (GravitinoEnv.getInstance() - .schemaDispatcher() - .schemaExists(NameIdentifier.of(ident.namespace().levels()))) { - EntityCombinedTopic combinedTopic = - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), - LockType.WRITE, - () -> importTopic(ident)); - return combinedTopic; - } return topic; } @@ -274,43 +277,47 @@ public boolean dropTopic(NameIdentifier ident) { : droppedFromCatalog; } - private EntityCombinedTopic importTopic(NameIdentifier identifier) { + private void importTopic(NameIdentifier identifier) { - EntityCombinedTopic combinedTopic = loadCombinedTopic(identifier); + EntityCombinedTopic topic = internalLoadTopic(identifier); - if (combinedTopic.imported()) { - return combinedTopic; + if (topic.imported()) { + return; } StringIdentifier stringId = null; try { - stringId = combinedTopic.stringIdentifier(); + stringId = topic.stringIdentifier(); } catch (IllegalArgumentException ie) { LOG.warn(FormattedErrorMessages.STRING_ID_PARSE_ERROR, ie.getMessage()); } long uid; if (stringId != null) { - // If the entity in the store doesn't match the external system, we use the data - // of external system to correct it. + // For Kafka topic, the uid is coming from topic UUID, which is always existed. + LOG.warn( + "The Topic uid {} existed but still needs to be imported, this could be happened " + + "when Topic is created externally without leveraging Gravitino. In this " + + "case, we need to store the stored entity to keep consistency.", + stringId); uid = stringId.id(); } else { - // If entity doesn't exist, we import the entity from the external system. + // This will not be happened for now, since we only support Kafka, and it always has an uid. uid = idGenerator.nextId(); } TopicEntity topicEntity = TopicEntity.builder() .withId(uid) - .withName(combinedTopic.name()) - .withComment(combinedTopic.comment()) + .withName(topic.name()) + .withComment(topic.comment()) .withNamespace(identifier.namespace()) .withAuditInfo( AuditInfo.builder() - .withCreator(combinedTopic.auditInfo().creator()) - .withCreateTime(combinedTopic.auditInfo().createTime()) - .withLastModifier(combinedTopic.auditInfo().lastModifier()) - .withLastModifiedTime(combinedTopic.auditInfo().lastModifiedTime()) + .withCreator(topic.auditInfo().creator()) + .withCreateTime(topic.auditInfo().createTime()) + .withLastModifier(topic.auditInfo().lastModifier()) + .withLastModifiedTime(topic.auditInfo().lastModifiedTime()) .build()) .build(); @@ -318,13 +325,11 @@ private EntityCombinedTopic importTopic(NameIdentifier identifier) { store.put(topicEntity, true); } catch (Exception e) { LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e); - throw new RuntimeException("Fail to access underlying storage"); + throw new RuntimeException("Fail to import topic entity to store.", e); } - - return combinedTopic; } - private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { + private EntityCombinedTopic internalLoadTopic(NameIdentifier ident) { NameIdentifier catalogIdent = getCatalogIdentifier(ident); Topic topic = doWithCatalog( @@ -333,10 +338,6 @@ private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { NoSuchTopicException.class); StringIdentifier stringId = getStringIdFromProperties(topic.properties()); - // Case 1: The topic is not created by Gravitino. - // Note: for Kafka catalog, stringId will not be null. Because there is no way to store the - // Gravitino - // ID in Kafka, therefor we use the topic ID as the Gravitino ID if (stringId == null) { return EntityCombinedTopic.of(topic) .withHiddenPropertiesSet( @@ -352,12 +353,10 @@ private EntityCombinedTopic loadCombinedTopic(NameIdentifier ident) { "GET", getStringIdFromProperties(topic.properties()).id()); - boolean imported = topicEntity != null; - return EntityCombinedTopic.of(topic, topicEntity) .withHiddenPropertiesSet( getHiddenPropertyNames( catalogIdent, HasPropertyMetadata::topicPropertiesMetadata, topic.properties())) - .withImported(imported); + .withImported(topicEntity != null); } } From 870670e85bbcce363d84d255f08535a9393d7fd3 Mon Sep 17 00:00:00 2001 From: Jerry Shao Date: Mon, 17 Jun 2024 20:47:36 +0800 Subject: [PATCH 13/15] change code --- .../gravitino/catalog/SchemaOperationDispatcher.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java index 273a7d0617a..c22cf5cef09 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java @@ -165,10 +165,6 @@ public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException { EntityCombinedSchema schema = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadSchema(ident)); - if (schema.imported()) { - return schema; - } - if (!schema.imported()) { TreeLockUtils.doWithTreeLock( NameIdentifier.of(ident.namespace().levels()), From 8e2a7d3d7811dbfb5236dc5db3c2830dcca29f99 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 17 Jun 2024 21:31:36 +0800 Subject: [PATCH 14/15] Polish the comment --- .../datastrato/gravitino/catalog/TableOperationDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java index 169b002e9a7..fd1cafa3869 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java @@ -431,7 +431,7 @@ private EntityCombinedTable internalLoadTable(NameIdentifier ident) { NoSuchTableException.class); StringIdentifier stringId = getStringIdFromProperties(table.properties()); - // Case 1: The table is not created by Gravitino or the backend storage does not support storing + // Case 1: The table is not created by Gravitino or the external system does not support storing // string identifier. if (stringId == null) { return EntityCombinedTable.of(table) From e661e566fbe342d68a102f0bbe9db3939f39b689 Mon Sep 17 00:00:00 2001 From: Jerry Shao Date: Tue, 18 Jun 2024 15:49:05 +0800 Subject: [PATCH 15/15] Address the comment --- .../com/datastrato/gravitino/catalog/EntityCombinedSchema.java | 2 ++ .../com/datastrato/gravitino/catalog/EntityCombinedTable.java | 2 ++ .../com/datastrato/gravitino/catalog/EntityCombinedTopic.java | 2 ++ 3 files changed, 6 insertions(+) diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java index aa7d575b82f..831daf6a5ac 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java @@ -29,6 +29,8 @@ public final class EntityCombinedSchema implements Schema { // Field "imported" is used to indicate whether the entity has been imported to Gravitino // managed storage backend. If "imported" is true, it means that storage backend have stored // the correct entity. Otherwise, we should import the external entity to the storage backend. + // This is used for tag/access control related purposes, only the imported entities have the + // unique id, and based on this id, we can label and control the access to the entities. private boolean imported; private EntityCombinedSchema(Schema schema, SchemaEntity schemaEntity) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java index 6478a260145..3edfe491390 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTable.java @@ -35,6 +35,8 @@ public final class EntityCombinedTable implements Table { // Field "imported" is used to indicate whether the entity has been imported to Gravitino // managed storage backend. If "imported" is true, it means that storage backend have stored // the correct entity. Otherwise, we should import the external entity to the storage backend. + // This is used for tag/access control related purposes, only the imported entities have the + // unique id, and based on this id, we can label and control the access to the entities. private boolean imported; private EntityCombinedTable(Table table, TableEntity tableEntity) { diff --git a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java index 3ddb4a20885..bf24cb74792 100644 --- a/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java +++ b/core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedTopic.java @@ -28,6 +28,8 @@ public class EntityCombinedTopic implements Topic { // Field "imported" is used to indicate whether the entity has been imported to Gravitino // managed storage backend. If "imported" is true, it means that storage backend have stored // the correct entity. Otherwise, we should import the external entity to the storage backend. + // This is used for tag/access control related purposes, only the imported entities have the + // unique id, and based on this id, we can label and control the access to the entities. private boolean imported; private EntityCombinedTopic(Topic topic, TopicEntity topicEntity) {