roryqi commented on code in PR #14540:
URL: https://github.com/apache/gravitino/pull/14540#discussion_r4488137511
##########
core/src/main/java/org/apache/gravitino/catalog/TableDispatcher.java:
##########
@@ -27,4 +31,17 @@
* 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 {
+ /**
+ * Imports an externally created table into Gravitino, or returns the
already imported entity.
+ * Catalog-reported audit fields take precedence over the supplied audit.
Existing entity audits
+ * are preserved. This operation does not create the table in the underlying
catalog.
Review Comment:
Updated in 0e7d651e04. The javadoc now limits preservation to existing
nonempty creation fields, describes conditional recovery after a competing
import, and explicitly notes that loading refreshes changed columns and their
modification audit. The column-refresh behavior is retained.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -793,6 +812,13 @@ private void importSchema(NameIdentifier identifier) {
LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", identifier, e);
throw new RuntimeException("Failed to import schema entity to the
store", e);
}
+ return EntityCombinedSchema.of(schema.schema(), schemaEntity)
+ .withHiddenProperties(
+ getMaskAndOmitKeys(
+ getCatalogIdentifier(identifier),
+ HasPropertyMetadata::schemaPropertiesMetadata,
+ schema.schema().properties()))
Review Comment:
Fixed in 0e7d651e04. `EntityCombinedSchema` now exposes its existing
classification within the package, and both the initial-import return path and
audit-recovery return path reuse it. They no longer reacquire a catalog lease
to classify properties after the write. The view import path also reuses its
classification. The schema secret-masking regression tests passed.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -310,35 +311,15 @@ public Schema createSchema(
*/
@Override
public Schema loadSchema(NameIdentifier ident) throws NoSuchSchemaException {
- // Load the schema and check if this schema is already imported.
- EntityCombinedSchema schema =
- TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadSchema(ident));
-
- if (!schema.imported()) {
- try {
- TreeLockUtils.doWithTreeLock(
- NameIdentifier.of(ident.namespace().levels()),
- LockType.WRITE,
- () -> {
- importSchema(ident);
- return null;
- });
- } catch (EntityAlreadyExistsException e) {
- // HA race: another Gravitino node concurrently imported this schema.
Reload from the
- // entity store to verify the entity stored by the winning node is
consistent.
- LOG.info(
- "Schema {} was concurrently imported by another node; reloading
from store.", ident);
- EntityCombinedSchema reloaded =
- TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadSchema(ident));
- if (!reloaded.imported()) {
- throw new UnsupportedOperationException(
- "Schema managed by multiple catalogs. This may cause unexpected
issues such as privilege conflicts. "
- + "To resolve: Remove all catalogs managing this schema,
then recreate one catalog to ensure single-catalog management.");
- }
- }
- }
+ return loadAndImportSchema(ident, AuditInfo.EMPTY);
+ }
- return schema;
+ /** {@inheritDoc} */
+ @Override
+ public Schema importSchema(NameIdentifier ident, AuditInfo auditInfo)
+ throws NoSuchSchemaException {
+ Preconditions.checkNotNull(auditInfo, "auditInfo must not be null");
Review Comment:
Fixed in 0e7d651e04. Replaced `checkNotNull` with `checkArgument(auditInfo
!= null, ...)` consistently in the schema, table and view import methods, and
updated the null-audit tests to expect `IllegalArgumentException`.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -676,10 +657,43 @@ private static Map<String, String>
propertiesForSchemaEntityAlter(
return newProps;
}
- private void importSchema(NameIdentifier identifier) {
+ private Schema loadAndImportSchema(NameIdentifier ident, AuditInfo
importAudit)
+ throws NoSuchSchemaException {
+ // Load the schema and check if this schema is already imported.
+ EntityCombinedSchema schema =
+ TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadSchema(ident));
+
+ if (!schema.imported()) {
+ try {
+ schema =
+ TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(ident.namespace().levels()),
+ LockType.WRITE,
+ () -> importSchemaFromCatalog(ident, importAudit));
+ } catch (EntityAlreadyExistsException e) {
+ // HA race: another Gravitino node concurrently imported this schema.
Reload from the
+ // entity store to verify the entity stored by the winning node is
consistent.
+ LOG.info(
+ "Schema {} was concurrently imported by another node; reloading
from store.", ident);
+ EntityCombinedSchema reloaded =
+ TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadSchema(ident));
+ if (!reloaded.imported()) {
+ throw new UnsupportedOperationException(
+ "Schema managed by multiple catalogs. This may cause unexpected
issues such as privilege conflicts. "
+ + "To resolve: Remove all catalogs managing this schema,
then recreate one catalog to ensure single-catalog management.");
Review Comment:
Fixed in 0e7d651e04. Split the exception message into shorter string
literals and applied the same formatting to the corresponding table/view paths.
Spotless checks passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
