[#10713] improvement(core,iceberg): Fix event ordering by standardizing dispatcher wiring by mehakmeet · Pull Request #10834 · apache/gravitino

mehakmeet · 2026-04-21T13:46:46Z

What changes were proposed in this pull request?

Standardized dispatcher wiring so that all object types follow the Hook(Event(Normalize(Manager))) pattern, matching the existing Policy/AccessControl wiring. This ensures CREATE events are published before SET_OWNER events for all object types.

GravitinoEnv: Flipped wiring for Metalake, Catalog, Schema, Table, Fileset, Topic, Model, Tag, Job dispatchers.
RESTService: Flipped wiring for Iceberg Table, View, Namespace dispatchers.
HookDispatchers: Ensured setOwner failures are gracefully handled via try-catch-log so that ownership failure does not cause creation failure.

Why are the changes needed?

Currently, most object types wrap HookDispatcher inside EventDispatcher (Event(Hook(Manager))). Since HookDispatcher auto-sets the creator as owner after creation, the SET_OWNER event is published before the CREATE event. This results in non-causal event ordering — external consumers see SET_OWNER for an object that hasn't been "created" yet from the event stream's perspective.

Policy already uses the correct pattern where HookDispatcher wraps EventDispatcher. This PR standardizes all object types to match.

Fix: #10713

Does this PR introduce any user-facing change?

No. This is an internal event ordering fix. The API behavior is unchanged. Events are now published in causal order (CREATE before SET_OWNER).

How was this patch tested?

Added setOwner-failure unit tests for all affected HookDispatchers to verify that creation succeeds even when setOwner throws:

Core (10 tests): TestMetalakeHookDispatcher, TestSchemaHookDispatcher, TestTableHookDispatcher, TestFilesetHookDispatcher, TestTopicHookDispatcher, TestModelHookDispatcher, TestTagHookDispatcher, TestPolicyHookDispatcher, TestAccessControlHookDispatcher, TestJobHookDispatcher

Iceberg (3 tests): TestIcebergTableHookDispatcher, TestIcebergViewHookDispatcher, TestIcebergNamespaceHookDispatcher

Tested in Gravitino dev environment. All core and iceberg tests pass:

./gradlew :core:test :iceberg:iceberg-rest-server:test -PskipITs

…ardizing dispatcher wiring Standardize dispatcher wiring in GravitinoEnv and RESTService so that all object types follow Hook(Event(Normalize(Manager))), matching the existing Policy/AccessControl pattern. This ensures CREATE events are published before SET_OWNER events for all object types. Changes: - GravitinoEnv: Flip wiring for Metalake, Catalog, Schema, Table, Fileset, Topic, Model, Tag, Job dispatchers. - RESTService: Flip wiring for Iceberg Table, View, Namespace dispatchers. - HookDispatchers: Ensure setOwner failures are gracefully handled via try-catch-log (no creation failure on ownership failure). Tests: - Added setOwner-failure tests for all core HookDispatchers. - Added setOwner-failure tests for all Iceberg HookDispatchers. - Tested in Gravitino dev environment. All core and iceberg tests pass: ./gradlew :core:test :iceberg:iceberg-rest-server:test -PskipITs

github-actions · 2026-04-21T15:27:09Z

Code Coverage Report

Overall Project	65.42% `-0.23%`	🟢
Files changed	45.21%	🔴

Module	Coverage
aliyun	1.73%	🔴
api	47.09%	🟢
authorization-common	85.96%	🟢
aws	1.1%	🔴
azure	2.6%	🔴
catalog-common	10.2%	🔴
catalog-fileset	80.02%	🟢
catalog-glue	75.36%	🟢
catalog-hive	81.83%	🟢
catalog-jdbc-clickhouse	79.06%	🟢
catalog-jdbc-common	43.93%	🟢
catalog-jdbc-doris	80.28%	🟢
catalog-jdbc-hologres	54.03%	🟢
catalog-jdbc-mysql	79.23%	🟢
catalog-jdbc-oceanbase	78.38%	🟢
catalog-jdbc-postgresql	82.05%	🟢
catalog-jdbc-starrocks	78.27%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	45.07%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	87.16%	🟢
catalog-lakehouse-paimon	77.71%	🟢
catalog-model	77.72%	🟢
cli	44.51%	🟢
client-java	77.63%	🟢
common	48.97%	🟢
core	81.82% `-1.08%`	🟢
filesystem-hadoop3	76.97%	🟢
flink	40.55%	🟢
flink-runtime	0.0%	🔴
gcp	14.2%	🔴
hadoop-common	10.39%	🔴
hive-metastore-common	46.14%	🟢
iceberg-common	55.2%	🟢
iceberg-rest-server	68.21% `-1.35%`	🟢
integration-test-common	0.0%	🔴
jobs	66.17%	🟢
lance-common	23.88%	🔴
lance-rest-server	57.84%	🟢
lineage	53.02%	🟢
optimizer	82.87%	🟢
optimizer-api	21.95%	🔴
server	85.89%	🟢
server-common	69.52%	🟢
spark	32.79%	🔴
spark-common	39.09%	🔴
trino-connector	34.27%	🔴

Files

Module	File	Coverage
core	FilesetHookDispatcher.java	85.71%	🟢
	SchemaHookDispatcher.java	83.33%	🟢
	TopicHookDispatcher.java	83.33%	🟢
	TableHookDispatcher.java	80.49%	🟢
	JobHookDispatcher.java	57.14%	🔴
	TagHookDispatcher.java	56.0%	🔴
	PolicyHookDispatcher.java	50.0%	🔴
	MetalakeHookDispatcher.java	47.22%	🔴
	CatalogHookDispatcher.java	35.85%	🔴
	ModelHookDispatcher.java	31.11%	🔴
	AccessControlHookDispatcher.java	18.92%	🔴
	GravitinoEnv.java	10.84%	🔴
iceberg-rest-server	IcebergViewHookDispatcher.java	92.41%	🟢
	IcebergTableHookDispatcher.java	76.0%	🟢
	IcebergNamespaceHookDispatcher.java	69.09%	🟢
	RESTService.java	0.0%	🔴

bharos

Overall +ve, some non-blocking comments to consider

bharos · 2026-04-21T21:48:19Z

    FilesetNormalizeDispatcher filesetNormalizeDispatcher =
-        new FilesetNormalizeDispatcher(filesetHookDispatcher, catalogManager);
-    this.filesetDispatcher = new FilesetEventDispatcher(eventBus, filesetNormalizeDispatcher);
+        new FilesetNormalizeDispatcher(filesetOperationDispatcher, catalogManager);


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

bharos · 2026-04-21T21:49:00Z

+      importSchema(context.catalogName(), createRequest.namespace());
+      IcebergOwnershipUtils.setSchemaOwner(
+          metalake,


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.

bharos · 2026-04-21T21:49:32Z

+      importTable(context.catalogName(), namespace, registerTableRequest.name());
+
+      // Set the owner of the registered table to the current user
+      IcebergOwnershipUtils.setTableOwner(


Similar comment as IcebergNamespaceHookDispatcher.createNamespace, please check if we should fix these

bharos · 2026-04-21T21:51:13Z

      accessControlDispatcher.addUser(ident.name(), PrincipalUtils.getCurrentUserName());
    }


addUser is NOT wrapped in try-catch — can still cause 500 error
Do we need similar treatment like setOwner here ?

This is a pre-existing issue, not introduced by this PR, but is now more visible since the event ordering change surfaces it.

bharos · 2026-04-21T21:52:04Z

+            Owner.Type.USER);
+      }
+    } catch (Exception e) {
+      LOG.warn("Fail to set owner for metalake {}, metalake exists without owner", ident, e);


nit : "Fail to set owner" → "Failed to set owner" on all the messages?

mchades · 2026-04-22T15:48:40Z

+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.namespace().level(0),
+            NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.TABLE),


With the new dispatcher chain order (HookDispatcher → EventDispatcher → NormalizeDispatcher → Manager), ident here is the raw caller input — it has not yet passed through NormalizeDispatcher. For TABLE scope, both the leaf name and the schema name inside ident.namespace() can be normalized, but the entity is stored under the normalized names. This setOwner call may therefore silently fail (exception swallowed by the catch block).

mchades · 2026-04-22T15:48:40Z

+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.namespace().level(0),
+            NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.SCHEMA),


Same normalization issue as in TableHookDispatcher#createTable. For SCHEMA scope, only the leaf (schema) name is affected — the namespace metalake.catalog is not normalized by NormalizeDispatcher.

mchades · 2026-04-22T15:48:40Z

+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.namespace().level(0),
+            NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.FILESET),


Same normalization issue as in TableHookDispatcher#createTable.

mchades · 2026-04-22T15:48:40Z

+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.namespace().level(0),
+            NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.TOPIC),


Same normalization issue as in TableHookDispatcher#createTable.

mchades · 2026-04-22T15:48:40Z

+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.namespace().level(0),
+            NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.MODEL),


Same normalization issue as in TableHookDispatcher#createTable.

mchades · 2026-04-22T15:48:40Z

+      OwnerDispatcher ownerManager = GravitinoEnv.getInstance().ownerDispatcher();
+      if (ownerManager != null) {
+        ownerManager.setOwner(
+            ident.name(),


(Pre-existing, unrelated to this PR) The first argument to setOwner should be the metalake name (ident.namespace().level(0)), not the model name (ident.name()).

mehakmeet added 2 commits April 21, 2026 17:38

fixing nits

63707f0

mehakmeet force-pushed the improvement/10713-event-ordering-dispatchers branch from e675c1f to 63707f0 Compare April 21, 2026 14:20

bharos reviewed Apr 21, 2026

View reviewed changes

roryqi requested review from mchades and yuqi1129 April 22, 2026 03:12

mchades reviewed Apr 22, 2026

View reviewed changes

Pattern	Entity Types
Hook → Normalize → Event → Manager	Metalake, Catalog, Schema
Hook → Event → Normalize → Manager	Table, Fileset, Topic, Model

		accessControlDispatcher.addUser(ident.name(), PrincipalUtils.getCurrentUserName());
		}

Conversation

mehakmeet commented Apr 21, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

github-actions Bot commented Apr 21, 2026

Code Coverage Report

Uh oh!

bharos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants