bharos commented on code in PR #10834:
URL: https://github.com/apache/gravitino/pull/10834#discussion_r3120484923
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceHookDispatcher.java:
##########
@@ -122,17 +134,24 @@ public LoadTableResponse registerTable(
RegisterTableRequest registerTableRequest) {
LoadTableResponse response = dispatcher.registerTable(context, namespace,
registerTableRequest);
- // Import the registered table into Gravitino's catalog so it exists as a
metadata object
- importTable(context.catalogName(), namespace, registerTableRequest.name());
-
- // Set the owner of the registered table to the current user
- IcebergOwnershipUtils.setTableOwner(
- metalake,
- context.catalogName(),
- namespace,
- registerTableRequest.name(),
- context.userName(),
- GravitinoEnv.getInstance().ownerDispatcher());
+ try {
+ // Import the registered table into Gravitino's catalog so it exists as
a metadata object
+ importTable(context.catalogName(), namespace,
registerTableRequest.name());
+
+ // Set the owner of the registered table to the current user
+ IcebergOwnershipUtils.setTableOwner(
Review Comment:
Similar comment as IcebergNamespaceHookDispatcher.createNamespace, please
check if we should fix these
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -593,29 +597,32 @@ private void initGravitinoServerComponents() {
FilesetOperationDispatcher filesetOperationDispatcher =
new FilesetOperationDispatcher(catalogManager, entityStore,
idGenerator);
- FilesetHookDispatcher filesetHookDispatcher =
- new FilesetHookDispatcher(filesetOperationDispatcher);
FilesetNormalizeDispatcher filesetNormalizeDispatcher =
- new FilesetNormalizeDispatcher(filesetHookDispatcher, catalogManager);
- this.filesetDispatcher = new FilesetEventDispatcher(eventBus,
filesetNormalizeDispatcher);
+ new FilesetNormalizeDispatcher(filesetOperationDispatcher,
catalogManager);
Review Comment:
The PR description claims all types follow Hook(Event(Normalize(Manager))),
but the actual wiring is:
Pattern | Entity Types
-- | --
Hook → Normalize → Event → Manager | Metalake, Catalog, Schema
Hook → Event → Normalize → Manager | Table, Fileset, Topic, Model
Both achieve CREATE before SET_OWNER, so this doesn't break the PR's goal.
However, the position of NormalizeDispatcher relative to EventDispatcher
affects whether events see pre-normalization or post-normalization input data.
This internal inconsistency could cause subtle surprises later.
We can use consistent pattern if that's possible
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceHookDispatcher.java:
##########
@@ -59,13 +64,20 @@ public CreateNamespaceResponse createNamespace(
IcebergRequestContext context, CreateNamespaceRequest createRequest) {
CreateNamespaceResponse response = dispatcher.createNamespace(context,
createRequest);
- importSchema(context.catalogName(), createRequest.namespace());
- IcebergOwnershipUtils.setSchemaOwner(
- metalake,
- context.catalogName(),
- createRequest.namespace(),
- context.userName(),
- GravitinoEnv.getInstance().ownerDispatcher());
+ try {
+ importSchema(context.catalogName(), createRequest.namespace());
+ IcebergOwnershipUtils.setSchemaOwner(
+ metalake,
Review Comment:
The try-catch wraps both importSchema and setSchemaOwner. If importSchema
fails, the namespace exists in the external catalog but is never registered in
Gravitino's entity store, and the failure is silently logged with a misleading
"set owner" message. These should be separate try-catch blocks, or importSchema
failure should propagate.
##########
core/src/main/java/org/apache/gravitino/hook/MetalakeHookDispatcher.java:
##########
@@ -71,14 +74,18 @@ public Metalake createMetalake(
accessControlDispatcher.addUser(ident.name(),
PrincipalUtils.getCurrentUserName());
}
Review Comment:
This is a pre-existing issue, not introduced by this PR, but is now more
visible since the event ordering change surfaces it.
##########
core/src/main/java/org/apache/gravitino/hook/MetalakeHookDispatcher.java:
##########
@@ -71,14 +74,18 @@ public Metalake createMetalake(
accessControlDispatcher.addUser(ident.name(),
PrincipalUtils.getCurrentUserName());
}
- // Set the creator as owner of the metalake.
- OwnerDispatcher ownerDispatcher =
GravitinoEnv.getInstance().ownerDispatcher();
- if (ownerDispatcher != null) {
- ownerDispatcher.setOwner(
- ident.name(),
- NameIdentifierUtil.toMetadataObject(ident,
Entity.EntityType.METALAKE),
- PrincipalUtils.getCurrentUserName(),
- Owner.Type.USER);
+ try {
+ // Set the creator as owner of the metalake.
+ OwnerDispatcher ownerDispatcher =
GravitinoEnv.getInstance().ownerDispatcher();
+ if (ownerDispatcher != null) {
+ ownerDispatcher.setOwner(
+ ident.name(),
+ NameIdentifierUtil.toMetadataObject(ident,
Entity.EntityType.METALAKE),
+ PrincipalUtils.getCurrentUserName(),
+ Owner.Type.USER);
+ }
+ } catch (Exception e) {
+ LOG.warn("Fail to set owner for metalake {}, metalake exists without
owner", ident, e);
Review Comment:
nit : "Fail to set owner" → "Failed to set owner" on all the messages?
##########
core/src/main/java/org/apache/gravitino/hook/MetalakeHookDispatcher.java:
##########
@@ -71,14 +74,18 @@ public Metalake createMetalake(
accessControlDispatcher.addUser(ident.name(),
PrincipalUtils.getCurrentUserName());
}
Review Comment:
addUser is NOT wrapped in try-catch — can still cause 500 error
Do we need similar treatment like setOwner here ?
--
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]
