jerryshao commented on code in PR #14540:
URL: https://github.com/apache/gravitino/pull/14540#discussion_r4480748198
##########
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(
Review Comment:
**major:** A namespace this request actually creates can still be stored
with a null creator. This is the opposite case from yuqi's ancestor comment.
`createNamespace` releases the branch lock before `importSchema(leaf,
alice)`. A concurrent `loadSchema(A)` imports A with `AuditInfo.EMPTY`, and
`insertSchema` then skips that row (`SchemaMetaService` L216-218). `newlyOwned`
is only passed to `setSchemaOwners`, so nothing writes Alice onto A. The old
`recordCreator` loop did that for every namespace this request created.
Could you persist the leaf audit only for namespaces in `newlyOwned`? If
such a row already exists with a null creator, please fill
`creator`/`createTime` without replacing a non-null creator. A hook-to-store
test covering `loadSchema(A)` before the leaf import of `A.B`, where A was
created by this request, would help.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -571,6 +585,9 @@ private EntityCombinedView importView(NameIdentifier ident)
throws NoSuchViewExc
.withLastModifier(catalogView.auditInfo().lastModifier())
.withLastModifiedTime(catalogView.auditInfo().lastModifiedTime())
.build();
+ if (audit.creator() == null) {
Review Comment:
**major:** View import can erase a creator that the initial import just
stored.
`importViewFromCatalog` always calls `store.put(viewEntity, true /*
overwrite */)` (L608), and `ViewMetaService.viewPOForOverwrite` replaces the
stored audit with the incoming one. Across nodes (tree locks are per process),
a `loadView` or a reconcile `importView` with `AuditInfo.EMPTY` can pass the
not-imported check before this put commits, and then overwrite Alice with an
empty creator. Schema and table inserts use `put(..., false)` and keep the
winning row.
Could you insert a new view without overwrite, and on
`EntityAlreadyExistsException` reload the row and fill only
`creator`/`createTime` when the stored creator is null? A two-thread store
test, where the creator import commits and then an overlapping empty load runs,
would cover this.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaDispatcher.java:
##########
@@ -89,4 +91,16 @@ default Schema createSchema(
throws NoSuchCatalogException, SchemaAlreadyExistsException {
throw new UnsupportedOperationException("Creating a schema with secrets is
not supported");
}
+ /**
Review Comment:
**nit:** Please add the usual blank line before this javadoc.
google-java-format puts a blank line between methods, so `spotlessCheck` will
flag it.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergViewHookDispatcher.java:
##########
@@ -184,9 +183,9 @@ public void renameView(IcebergRequestContext context,
RenameTableRequest renameV
/**
* Import a view into Gravitino's metadata catalog by loading it through the
ViewDispatcher.
*
- * <p>This method calls {@link ViewDispatcher#loadView} which delegates to
the underlying
- * catalog's ViewCatalog implementation. This ensures Gravitino is aware of
the view and can apply
- * authorization policies to it.
+ * <p>This method calls {@link ViewDispatcher#importView(NameIdentifier,
AuditInfo)} which
Review Comment:
**nit:** This helper doesn't delegate to the underlying `ViewCatalog`.
`importView` loads the existing view and registers it in the entity store.
Could the javadoc say that?
--
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]
