yuqi1129 commented on code in PR #14540:
URL: https://github.com/apache/gravitino/pull/14540#discussion_r4479053516
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableHookDispatcher.java:
##########
@@ -256,15 +256,11 @@ private void importTableAndSetOwner(
IcebergRequestContext context, Namespace namespace, String tableName) {
// Validate request metadata before the backend operation. An unexpected
import failure must
// propagate without deleting a table that another client may already have
updated or recreated.
- importTableEntity(context.catalogName(), namespace, tableName);
- IcebergEntityAuditUtils.recordCreator(
- IcebergIdentifierUtils.toGravitinoTableIdentifier(
- metalake,
- context.catalogName(),
- TableIdentifier.of(namespace, tableName),
- HierarchicalSchemaUtil.schemaSeparator()),
- Entity.EntityType.TABLE,
- context.userName());
+ importTableEntity(
+ context.catalogName(),
+ namespace,
+ tableName,
+
AuditInfo.builder().withCreator(context.userName()).withCreateTime(Instant.now()).build());
Review Comment:
[Important] Preserve the creator when an ordinary load wins the initial
import.
There is a window between the backend create becoming visible and this
import:
1. Alice creates the external table.
2. Another request calls ordinary `loadTable`, importing it with
`AuditInfo.EMPTY`.
3. This `importTable(..., alice)` sees an already-imported entity and skips
the fallback merge.
The create succeeds, but the stored creator/createTime remain empty.
Previously, the subsequent `recordCreator` filled the missing fields.
Schema/view imports have the same already-imported branch.
Could we retain the single-write cold path while conditionally filling
missing creation audit when a competing import has already stored the same
external object? Existing nonempty creators must remain intact, and the
solution needs to cover cross-node imports as well as local locking. Please add
a create-hook-to-storage regression test for this ordering.
Validation: I ran a deterministic probe using the real Iceberg table hook,
core TableOperationDispatcher and an in-memory entity store, inserting an
ordinary load before the hook import. It fails here (`expected alice but was
null`) and passes with the base version's hook + creator utility. Schema/view
symmetry was checked statically; no multi-node HTTP test was run.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceHookDispatcher.java:
##########
@@ -90,21 +92,18 @@ public CreateNamespaceResponse createNamespace(
});
// SchemaMetaService.insertSchema splits the leaf's logical name and
auto-creates a
// Gravitino entity row for each ancestor, so a single leaf import covers
the branch.
+ // Newly inserted ancestor rows inherit the leaf audit in that same write.
// Failures propagate intentionally: swallowing would leave a namespace in
Iceberg
// that Gravitino doesn't know about. Do not drop the namespace on import
failure: another
// client may already have created objects in it.
- importSchema(catalogName, leaf);
+ importSchema(
+ catalogName,
+ leaf,
+
AuditInfo.builder().withCreator(context.userName()).withCreateTime(Instant.now()).build());
Review Comment:
[Important] A newly inserted ancestor row does not necessarily represent a
newly created external namespace.
For example, Iceberg already contains `A`, but Gravitino has not imported
it. Alice creates `A:B`. `getMissingAncestors` correctly excludes `A`, yet
importing the leaf with Alice's audit makes `SchemaMetaService.insertSchema`
create the missing `A` row with the same audit. This incorrectly records
Alice/current time as the creator/creation time of an existing namespace. The
old `newlyOwned` loop excluded that ancestor. Ownership assignment is still
scoped correctly; the regression is in audit data.
Could we carry the set of actually created external namespaces into the
audit persistence logic, so importing an existing ancestor preserves its source
audit or unknown values? Please cover "external A exists, store A is absent"
through the hook and real persistence layer.
Validation: traced this hook through core import and SchemaMetaService's
ancestor insertion; the H2 test executes and asserts inheritance of the leaf
audit. The existing nested hook test mocks import and no longer checks that
existing A receives no creator. Catalog-owner authorization can short-circuit
without first importing A, so this state is reachable. A complete HTTP-to-H2
test was not run.
##########
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:
[Nit] Could we reuse the hidden-property classification already carried by
the loaded schema when replacing its entity?
`internalLoadSchema` has already classified these source properties. This
new return path acquires another catalog lease, invokes the connector's schema
property metadata provider and computes the classification again, after the
store write has succeeded. A helper that preserves the existing classification
would avoid this repeated work and the additional post-write failure point,
similar to the table path reusing `hiddenProperties()`.
Validation: compared the base/head import paths and traced
getMaskAndOmitKeys through CatalogManager. A warm catalog can be served from
cache, so this is not necessarily an extra SQL query. Secret-masking tests
passed; no benchmark or call-count test was run.
##########
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:
[Nit] Could we narrow "Existing entity audits are preserved" to the creation
fields and document the load-side column refresh?
`importTable` still calls `updateColumnsIfNecessaryWhenLoad`. If external
columns changed, it updates the stored columns and
lastModifier/lastModifiedTime using the current principal/time. This behavior
is needed by the IRC schema-commit path, but contradicts the blanket
audit-preservation promise here.
Validation: traced importTable -> loadAndImportTable ->
updateColumnsIfNecessaryWhenLoad and the IRC caller; the relevant core/IRC
tests passed. This is a documentation issue, not a request to remove modifier
updates.
--
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]
