jerryshao commented on code in PR #13791:
URL: https://github.com/apache/gravitino/pull/13791#discussion_r4266745801
##########
core/src/main/java/org/apache/gravitino/listener/SemanticModelEventDispatcher.java:
##########
@@ -131,6 +140,54 @@ public SemanticModel createSemanticModel(
}
}
+ @Override
+ public SemanticModel importOssieDocument(
+ Namespace namespace,
+ OssieDocument document,
+ Consumer<SemanticModelDefinition> sourceValidator) {
+ String user = PrincipalUtils.getCurrentUserName();
+ OssieFormat format = document == null ? null : document.format();
+ try {
+ eventBus.dispatchEvent(new ImportSemanticModelPreEvent(user, namespace,
format));
+ SemanticModel semanticModel =
+ dispatcher.importOssieDocument(namespace, document, sourceValidator);
+ eventBus.dispatchEvent(
+ new ImportSemanticModelEvent(
+ user,
+ NameIdentifier.of(namespace, semanticModel.name()),
+ format,
+ new SemanticModelInfo(semanticModel)));
+ return semanticModel;
+ } catch (Exception e) {
+ eventBus.dispatchEvent(new ImportSemanticModelFailureEvent(user,
namespace, format, e));
+ throw e;
+ }
+ }
+
+ @Override
+ public SemanticModel importSemanticModel(
+ NameIdentifier ident,
+ @Nullable String comment,
+ SemanticModelDefinition definition,
+ Map<String, String> properties) {
+ throw new UnsupportedOperationException(
Review Comment:
[Question] This rejection closes the event-less parsed-import path on the
top-level dispatcher, which is what my earlier comment asked for. The same
shape still exists one layer down, and it is not introduced by this PR:
`GravitinoEnv.initSemanticModelDispatcher`
(`core/src/main/java/org/apache/gravitino/GravitinoEnv.java:1018-1024`) builds
`internalSemanticModelDispatcher` as Normalize -> Operation, with no hook and
no event dispatcher, and exposes it under the same `SemanticModelDispatcher`
type (`GravitinoEnv.java:423-429`). On that instance both
`importSemanticModel(...)` and the inherited `importOssieDocument(ns, doc,
definition -> {})` write a model with no import event, no audit record and no
owner. The full server overwrites `semanticModelDispatcher` with the event
chain at `GravitinoEnv.java:1150-1163`, and the only internal caller today is a
read
(`core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:290`,
`semanticModelExists`), so nothing is broken; `i
nitializeMetadataComponents`, where `semanticModelDispatcher()` stays the
hook-less chain, is used only by core tests.
Worth making that explicit - a sentence on
`internalSemanticModelDispatcher()` saying it is for reads and existence
checks, or rejecting the write entry points there too? Not blocking; just the
one remaining way to import without the events this PR adds.
Verified by: read both init paths in `GravitinoEnv` (1018-1024 and
1150-1163), traced which initialize* method calls each
(`initializeFullComponents` -> `initGravitinoServerComponents` ->
`initPublicMetadataDispatchers`), and grepped every caller of
`internalSemanticModelDispatcher()` and of `initializeMetadataComponents`.
##########
core/src/main/java/org/apache/gravitino/catalog/SemanticModelDispatcher.java:
##########
@@ -18,41 +18,161 @@
*/
package org.apache.gravitino.catalog;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Consumer;
+import javax.annotation.Nullable;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
+import org.apache.gravitino.exceptions.IllegalSemanticModelException;
+import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NoSuchSemanticModelException;
+import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException;
import org.apache.gravitino.semantic.OssieDocument;
import org.apache.gravitino.semantic.OssieDocumentConverter;
-import
org.apache.gravitino.semantic.OssieDocumentConverter.ImportedSemanticModel;
+import
org.apache.gravitino.semantic.OssieDocumentConverter.ParsedSemanticModel;
import org.apache.gravitino.semantic.OssieFormat;
import org.apache.gravitino.semantic.SemanticModel;
-import org.apache.gravitino.semantic.SemanticModelCatalog;
+import org.apache.gravitino.semantic.SemanticModelChange;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
-/** A dispatcher specialization for schema-scoped Semantic Model operations. */
-public interface SemanticModelDispatcher extends SemanticModelCatalog {
+/** Dispatches schema-scoped Semantic Model operations through the server
pipeline. */
+public interface SemanticModelDispatcher {
Review Comment:
[Nit] This is now the only dispatcher interface in the repo that is not tied
to its public catalog API: `TableDispatcher.java:30`, `ViewDispatcher.java:29`,
`ModelDispatcher.java:23`, `FunctionDispatcher.java:29`,
`TopicDispatcher.java:30` and `FilesetDispatcher.java:30` all extend theirs.
Dropping `extends SemanticModelCatalog` does remove the inherited 2-arg no-op
import, which is the point of the commit, but it also removes the compile-time
link between the public API and the chain the REST layer is injected with
(`server/src/main/java/org/apache/gravitino/server/GravitinoServer.java:189-191`):
if `SemanticModelCatalog` later gains a method, nothing here fails and the
server chain simply will not have it.
The two contracts are identical today, and the `semanticModelExists` default
body is now duplicated verbatim
(`api/src/main/java/org/apache/gravitino/semantic/SemanticModelCatalog.java:59-66`
vs lines 66-73 here), along with its test
(`api/src/test/java/org/apache/gravitino/semantic/TestSemanticModelCatalog.java:35-41`
and
`core/src/test/java/org/apache/gravitino/catalog/TestSemanticModelDispatcher.java:68-78`).
Suggestion: add one reflection guard in `TestSemanticModelDispatcher`, in
the style of the structural tests already added there, asserting that every
public method of `SemanticModelCatalog` has a declaration with the same name
and parameter types on this interface. That keeps the decoupling while catching
a future API addition.
Verified by: compared the declared method sets of both interfaces (they
match 1:1 plus `importSemanticModel`), read all six sibling dispatcher
interfaces, and grepped the repo for `SemanticModelCatalog` (only the api
module, `ManagedSemanticModelOperations.java:49`, and the new test).
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java:
##########
@@ -134,7 +145,8 @@ public HttpServletRequest get() {
}
}
- private final SemanticModelDispatcher dispatcher =
mock(SemanticModelDispatcher.class);
+ private final SemanticModelDispatcher dispatcher =
mock(SemanticModelOperationDispatcher.class);
Review Comment:
[Nit] The REST test now mocks the core concrete class
`SemanticModelOperationDispatcher` so `doCallRealMethod()` can supply real
`importSemanticModel` and `exportOssieDocument` bodies (lines 207-210). The
consequence is that a server-module test asserts core-internal behaviour: the
`verify(dispatcher).createSemanticModel(...)` assertions around lines 1090-1098
pass only because `SemanticModelOperationDispatcher.importSemanticModel`
aliases create
(`core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java:120-127`).
If that stops being a plain alias, these REST tests break for a reason
unrelated to REST.
Suggestion: use a small hand-written `SemanticModelDispatcher` test double
in this class (import -> create, export -> load plus convert) and keep the
assertions on `importOssieDocument` / `importSemanticModel`, so the server test
depends on the interface rather than on a core implementation class.
Verified by: read the binder at lines 176-191, where the chain under test is
`new SemanticModelEventDispatcher(eventBus, dispatcher)` with Normalize and
Hook absent, and the real `importSemanticModel` / `exportOssieDocument` bodies
in `SemanticModelOperationDispatcher`.
--
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]
