Copilot commented on code in PR #3138:
URL: https://github.com/apache/hugegraph/pull/3138#discussion_r3759947335
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
Review Comment:
The PR description says this is a one-line parity fix (swap async
`eventHub.notify(...)` to a wait variant), but the actual change set introduces
new APIs (`notifySync`), new failure semantics (create now throws if any
listener failed), and also changes the local create path and drop path
behavior. Please either (mandatory) update the PR description to reflect the
broader behavioral changes, or reduce the scope to the stated parity fix to
avoid surprising reviewers/releasers.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1193,18 +1202,20 @@ private HugeGraph createGraphLocal(HugeConfig config,
String name) {
// Init graph and start it
graph.create(this.graphsDir, this.globalNodeRoleInfo);
+
Review Comment:
The PR description says this is a one-line parity fix (swap async
`eventHub.notify(...)` to a wait variant), but the actual change set introduces
new APIs (`notifySync`), new failure semantics (create now throws if any
listener failed), and also changes the local create path and drop path
behavior. Please either (mandatory) update the PR description to reflect the
broader behavioral changes, or reduce the scope to the stated parity fix to
avoid surprising reviewers/releasers.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1342,19 +1353,37 @@ public HugeGraph createGraph(String graphSpace, String
name, String creator,
graph.updateTime(timeStamp);
String graphName = spaceGraphName(graphSpace, name);
+ this.graphs.put(graphName, graph);
+
+ /*
+ * Let gremlin server and rest server context add graph before the
+ * graph is published, so that a failed local binding can't leave the
+ * graph behind in meta for the other servers to converge on
+ */
+ try {
+ this.notifyAndWaitEvent(Events.GRAPH_CREATE, graph);
+ } catch (Throwable e) {
+ this.graphs.remove(graphName);
+ try {
Review Comment:
The PR description says this is a one-line parity fix (swap async
`eventHub.notify(...)` to a wait variant), but the actual change set introduces
new APIs (`notifySync`), new failure semantics (create now throws if any
listener failed), and also changes the local create path and drop path
behavior. Please either (mandatory) update the PR description to reflect the
broader behavioral changes, or reduce the scope to the stated parity fix to
avoid surprising reviewers/releasers.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/ContextGremlinServer.java:
##########
Review Comment:
The `getX() != null` checks don’t make the removals race-safe: another
thread can remove the binding after the check but before `removeX()`, and
`removeX()` may still throw. Consider making removals idempotent by catching
the specific exception `removeGraph/removeTraversalSource` throws when missing
(or enhancing the underlying manager to provide `removeIfPresent` semantics)
rather than using check-then-act.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1771,10 +1800,60 @@ private void listenMetaChanges() {
this.metaManager.listenGraphClear(ConsumerWrapper.wrap(this::graphClearHandler));
}
+ /**
+ * Notify the listeners of `event` and wait for them to finish, failing if
+ * any listener did not complete successfully.
+ * <p>
+ * EventHub swallows every throwable raised by a listener and resolves the
+ * future with the number of listeners that returned normally, so waiting
+ * alone doesn't prove that the graph was registered. Comparing the
+ * notified count with the registered listener count detects the swallowed
+ * failure and lets the caller fail instead of returning a graph that is
+ * missing from the rest/gremlin server context.
+ */
Review Comment:
This success/failure detection can spuriously fail if the listener sets
change between computing `expected` and iterating in `notifySync()` (e.g.,
concurrent listener registration/unregistration). A more robust approach is to
have `EventHub` return both attempted and succeeded counts from a single
iteration (or return a small result object), so `GraphManager` doesn’t need a
separate precomputed `expected` based on sizes.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
