roryqi commented on code in PR #14540:
URL: https://github.com/apache/gravitino/pull/14540#discussion_r4488136358
##########
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:
Fixed in 0e7d651e04. Schema, table and view imports now conditionally fill
creation fields when a competing import has already stored a row with no
creator. The store updater rechecks both the entity ID and the current creator,
preserving an existing creator, creation time and modification fields. The cold
import still writes the audit in its initial insert.
Added `testTableHookFillsAuditAfterOrdinaryLoad`, which runs the real create
hook and core dispatcher against H2, with an ordinary load winning before the
hook import. Corresponding schema/view recovery tests are included. All 202
selected Core tests and 660 Iceberg REST non-IT tests passed.
##########
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:
Fixed in 0e7d651e04. The hook now imports ancestors from outermost to
innermost, passing the request audit only for namespaces in `newlyOwned`.
Pre-existing external ancestors retain their source audit or unknown values.
Storage-generated ancestor rows no longer inherit the leaf audit.
`testNamespaceHookAuditsOnlyCreatedNamespaces` exercises the hook, core
dispatcher and H2 persistence with an existing external parent absent from
storage, covering both a reported source audit and unknown creation fields, as
well as a parent created by this request.
--
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]
