fix(server): wait for GRAPH_CREATE event when creating graph on PD path - #3138
fix(server): wait for GRAPH_CREATE event when creating graph on PD path#3138bitflicker64 wants to merge 2 commits into
Conversation
The PD-backed createGraph fired GRAPH_CREATE without awaiting it, so the REST 200 could be written before ContextGremlinServer injected the graph into the Gremlin global bindings, and an immediate Gremlin/Cypher request to the creating server could fail with "Could not rebind [g]". createGraphLocal already waits via notifyAndWaitEvent; this applies the same call on the PD path.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3138 +/- ##
============================================
- Coverage 39.18% 32.42% -6.76%
Complexity 264 264
============================================
Files 770 770
Lines 65779 65811 +32
Branches 8726 8728 +2
============================================
- Hits 25774 21341 -4433
- Misses 37244 42023 +4779
+ Partials 2761 2447 -314 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The PD create path now waits for GRAPH_CREATE listeners, but listener failures remain non-fatal and the current head has a failing codecov/project check. Evidence: EventHub.notify() catches listener Throwable at hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/event/EventHub.java:200-205; final gate reports codecov/project FAILURE.
|
|
||
| // Let gremlin server and rest server context add graph | ||
| this.eventHub.notify(Events.GRAPH_CREATE, graph); | ||
| this.notifyAndWaitEvent(Events.GRAPH_CREATE, graph); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, fixed in 9a4ac02. EventHub.notify resolves with the count of listeners that returned normally, so notifyAndWaitEvent now compares that against the registered listener count and fails the create when one is missing.
Rolling back addGraphConfig / notifyGraphAdd would let this replica's binding failure delete a graph the others bound fine, so the graph is instead bound locally before it is published to meta; the failure path unregisters and closes it, like a failed backend init.
The count is an inference, not the real exception. Happy to switch to verifying the bindings directly, or to having EventHub surface listener failures, if you prefer either.
|
|
||
| // Let gremlin server and rest server context add graph | ||
| this.eventHub.notify(Events.GRAPH_CREATE, graph); | ||
| this.notifyAndWaitEvent(Events.GRAPH_CREATE, graph); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Agreed, fixed in 9a4ac02: bounded future.get(30s), and InterruptedException is caught on its own and restores the interrupt status before failing. The drop path gets the same bounded wait but stays lenient, since the data is already gone when the event fires and TinkerPop's removeGraph throws for a graph the Gremlin server never bound.
30s is a constant rather than an option, as ServerOptions has nothing comparable; happy to promote it if you want it tunable.
One call I would rather leave to you: a timeout currently fails the create, so a merely slow listener breaks a create that actually worked. Treating a timeout as unknown (log loudly, do not fail) is the alternative.
Waiting for the GRAPH_CREATE future was not enough to prove that the graph was actually registered: EventHub swallows every throwable raised by a listener and resolves the future with the number of listeners that returned normally, so a listener that blew up looked exactly like a successful one. The create now compares the notified count with the registered listener count and fails when a listener did not complete. The wait is also bounded now instead of blocking forever, and an InterruptedException restores the thread's interrupt status before the failure is reported. Ordering is fixed along with it. On the PD path the graph is bound in the local gremlin/rest server context before its config is written to meta and broadcast, so a failed binding cannot leave a graph behind in meta for the other servers to converge on. A binding failure now unregisters the graph locally and closes it, the same cleanup a failed backend init already does, rather than dropping data that other servers may have bound successfully. On the local path the notify moved inside the existing try, which now also unregisters the graph before dropping it, so a failed binding leaves no closed graph behind in the context. The drop path keeps the lenient behaviour: the data is already gone when the event fires, so failing the request cannot undo anything and the listener state may legitimately be absent already.
|
Both points addressed in 9a4ac02, details in the inline replies. Two things left out on purpose:
There is no automated coverage for the new failure semantics: nothing exercises
|
Purpose of the PR
In distributed mode (PD + HStore),
POST /graphspaces/{space}/graphs/{name}returns 200 before the creating server has actually bound the new graph into its embedded Gremlin server.GraphManager.createGraph(PD path) firesGRAPH_CREATEviathis.eventHub.notify(...), which is asynchronous, so the REST response can be written beforeContextGremlinServerinjects the graph and itsTraversalSourceinto the Gremlin global bindings. An immediate follow-up Gremlin/Cypher request to the same server then fails with HTTP 400:Could not rebind [g] to [__g_<name>] as [__g_<name>] could not be found in the Graph or TraversalSource global bindings.The local path already handles this correctly:
createGraphLocalcallsthis.notifyAndWaitEvent(Events.GRAPH_CREATE, graph), which blocks on the event future before returning. The PD path simply lost that parity.Main Changes
GraphManager.createGraph(PD path): replacethis.eventHub.notify(Events.GRAPH_CREATE, graph)withthis.notifyAndWaitEvent(Events.GRAPH_CREATE, graph), matching whatcreateGraphLocalalready does.GRAPH_CREATElisteners (Gremlin bindings injection included). Cross-replica convergence (other servers picking the graph up via the PD/meta watch) is intentionally out of scope here; that is phase 2/3 of [Feature] Orchestrate graph creation through PD: new graphs are not consistently available across Server replicas ("Could not rebind [g]") #3137.One question for reviewers, as raised in #3137:
notifyAndWaitEvent(GraphManager.java, around line 1774) waits on the event future but swallows listener failures with only aLOG.warn, so a failed bindings injection would still return 200. Should a listener failure fail the create instead? I left that behavior unchanged here to keep this PR a pure parity fix withcreateGraphLocal.Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need