Skip to content

fix(server): bind locally loaded graphs in Gremlin Server - #3351

Merged
imbajin merged 5 commits into
apache:masterfrom
byteayan:fix/server-gremlin-bind-local-graphs
Oct 16, 2026
Merged

imbajin merged 5 commits into
apache:masterfrom
byteayan:fix/server-gremlin-bind-local-graphs

Conversation

@byteayan

@byteayan byteayan commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

Gremlin Server opens the graphs in conf/graphs itself, once, inside HugeGremlinServer.prepare(). TinkerPop's DefaultGraphManager logs and drops any graph whose open throws. That open runs before the REST GraphManager exists, so it also runs before the pd.stores_wait_timeout wait. When PD is briefly unreachable at that point, the Gremlin open fails. A few seconds later REST opens the same graph successfully, after its wait. GraphManager.loadGraph(String, String) then stored the graph without firing GRAPH_CREATE, which is the only way ContextGremlinServer binds a graph after startup. The Server answered REST and passed readiness, and every Gremlin call on that graph failed with Could not rebind [DEFAULT-hugegraph] until the process restarted.

Graphs created through the API or loaded from PD meta don't have this problem, because createGraph() and createGraphLocal() already notify GRAPH_CREATE.

Main Changes

  • GraphManager.loadGraph() calls a new bindLocalGraph(), which fires GRAPH_CREATE for the loaded graph through the same notifyEvent() the create paths use. This resolves the TODO from chore: add TODO breadcrumbs for the Helm chart's server workarounds #3260.
  • In the normal case Gremlin's open succeeded and the rebind is harmless. Both opens go through the same gremlin.graph factory, HugeFactory caches graphs by name, and HugeFactoryAuthProxy caches one proxy per graph, so Gremlin is handed the instance it already holds. injectGraph() overwrites the binding, the __g_ traversal source and the global binding, and it runs before startPrepared(), so no request can see a half-updated binding.
  • The event is skipped, with a WARN, when the graph's spaceGraphName() differs from DEFAULT-<conf file name>: the store value differs from the file name, or graphspace isn't DEFAULT. Gremlin binds that file as DEFAULT-<file>, so firing the event would add a second binding and a second REST entry under the other name.
  • loadGraph()'s assert that the graph's config file matches the loaded path now skips the check when the config has no file. That is the case when Gremlin opened the graph first, and under -ea the assert threw before the bind.
  • A failed bind is logged at ERROR and the graph stays loaded in REST. Rethrowing would end up in the catch (Throwable) in loadGraphsFromLocal() with the same result, and failing startup over it is a separate decision.
  • reload() drops graphs with GRAPH_DROP, which removes their Gremlin binding, and the local reload never brought it back. It does now.
  • helm/hugegraph/docs/operations.md: the Could not rebind row now says which log lines to check on images with this fix. The per-Pod Gremlin check in helm test stays, since older images still need it.

Verifying these changes

  • Need tests and can be verified as follows:
    • New GraphManagerLocalGraphBindTest, registered in UnitTestSuite, uses the memory backend and needs no PD. It checks that a local load fires GRAPH_CREATE once, and that loading again keeps the same instance. A graph whose store name differs from its file name is not rebound. With a real ContextGremlinServer and no static graph (the failed-open case), the graph ends up bound along with its __g_ traversal source. When Gremlin opened the graph itself, the rebind still fires and its instance and __g_ traversal source stay the ones REST holds. A failed bind leaves the graph loaded in REST.
    • With the bindLocalGraph() call removed, 5 of the 6 tests fail; the name-mismatch case expects no bind.
    • mvn test -pl hugegraph-server/hugegraph-test -P unit-test: 1171 tests, 0 failures.
    • run-api-test.sh rocksdb (packaged Server with auth, as in CI): 189 tests, 0 failures, and the runtime log check passes.
    • Not reproduced end to end: the original failure needs a 3 PD + 3 Store cluster rolled during Server start and showed up in 4 of 12 starts in the issue.

Does this PR potentially affect the following parts?

  • Dependencies (add/update license info & regenerate_known_dependencies.sh)
  • Modify configurations
  • The public API
  • Other affects (typed here): Server startup now offers each graph in conf/graphs to Gremlin Server a second time; graphs Gremlin already holds are unchanged
  • Nope

Documentation Status

  • Doc - TODO: required documentation is pending; complete it before merging.
  • Doc - Done: documentation is included here or linked below.
  • Doc - No Need: no user-visible documentation is affected.

Documentation files in this PR or paired hugegraph-doc PR:

  • helm/hugegraph/docs/operations.md

Conceptual overview

Offer locally loaded graphs to Gremlin through GRAPH_CREATE after name checks, keeping the REST graph after binding failure

Concept illustration: re-offer locally loaded graphs to Gremlin through GRAPH_CREATE after name checks; failed binding keeps the REST graph. Not end-to-end test evidence.

Gremlin Server opens the graphs in conf/graphs on its own, once, while it
is being prepared, and drops any graph whose open fails there. With PD
briefly unreachable at that moment, the REST GraphManager still opened
the graph a few seconds later, after its PD readiness wait, but never
told Gremlin about it: loadGraph() stored the graph without firing
GRAPH_CREATE, unlike the create paths. The Server then answered REST and
passed readiness while every Gremlin call on that graph failed with
"Could not rebind" until the process restarted.

loadGraph() now offers each locally loaded graph to Gremlin through the
same GRAPH_CREATE event the create paths use. When Gremlin's own open
succeeded, both opens return the same cached instance, so the rebind is
harmless. reload() also gets its Gremlin binding back this way, since
dropping a graph removed it. A graph whose spaceGraphName differs from
DEFAULT-<conf file name> (a different store name or a non-DEFAULT
graphspace) is skipped with a warning so it is not bound under a second
name, and a failed bind is logged without unloading the graph from REST.

Fixes apache#3228
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 57.86%. Comparing base (92af2f8) to head (5419eaa).
⚠️ Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...n/java/org/apache/hugegraph/core/GraphManager.java 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3351      +/-   ##
============================================
+ Coverage     54.31%   57.86%   +3.55%     
+ Complexity     2250      130    -2120     
============================================
  Files           770      517     -253     
  Lines         67952    49910   -18042     
  Branches       9213     7332    -1881     
============================================
- Hits          36905    28881    -8024     
+ Misses        27470    18144    -9326     
+ Partials       3577     2885     -692     
Flag Coverage Δ
hstore 35.57% <38.46%> (?)
server_memory 52.75% <92.30%> (?)
server_rocksdb 42.25% <38.46%> (?)

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Score 7/10. The production change reads correctly: bindLocalGraph() fires GRAPH_CREATE through notifyEvent() after loadGraph() stores the graph, the DEFAULT-<file> guard keeps the event from adding a second Gremlin or REST name, and a failed bind is contained. One Important problem in the new tests: testGraphOpenedByGremlinKeepsItsInstance, the only test for the normal case where Gremlin already holds the graph, never reaches bindLocalGraph(). loadGraph() throws a NullPointerException before the bind, loadGraphsFromLocal() logs and swallows it, and both assertions pass anyway (details inline). Evidence: read the full diff 4b77b7b..85f4de9 (4 files) in a scratch worktree, together with GraphManager (constructor, init(), reload(), loadGraphsFromLocal(), loadGraph(), createGraphLocal(), listenChanges(), notifyEvent()), ContextGremlinServer (injectGraph(), removeGraph(), injectTraversalSource()), HugeGremlinServer.prepare(), the HugeGraphServer startup order, HugeFactory.open(), HugeFactoryAuthProxy.open() and HugeConfig(Configuration). The exact-head build-server / Server memory tests job log shows the NPE from this test, and its UnitTestSuite run passes (1170 tests). The only blocking check failure, pd_store / hstore, is an Apache RAT failure on two date-rolled bolt log files under hugegraph-store/apache-hugegraph-store-1.8.0/logs/, which is not caused by this diff. I did not run the tests locally or reproduce the PD restart scenario.

Add a case where a GRAPH_CREATE listener fails: the error is logged and
the graph stays loaded in REST. Also drop the instanceof HugeGraph guard
in bindLocalGraph(); loadGraph() already casts the graph's configuration
to HugeConfig, so only HugeGraph instances reach it.
When Gremlin Server opens a conf/graphs file first, HugeFactory caches
the graph with a HugeConfig built from a Configuration, which has no
file. The REST load of the same path gets that cached instance back, and
the assert comparing its config file with the loaded path threw a
NullPointerException under -ea before bindLocalGraph() ran.
loadGraphsFromLocal() logged and swallowed it, so the unit test for this
normal case passed without the rebind ever happening.

The assert now skips the comparison when the config has no file, and
the test checks that GRAPH_CREATE fired and that the __g_ traversal
source still belongs to the instance Gremlin opened.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: Score 9/10. At 664eb9a the one point from the earlier review at 85f4de9 is fixed: the loadGraph() assert now skips the comparison when a Gremlin-opened graph's config has no file, so testGraphOpenedByGremlinKeepsItsInstance reaches bindLocalGraph() and checks the rebind and the __g_ source. bindLocalGraph() fires GRAPH_CREATE before Gremlin Server starts, the DEFAULT-<file> guard keeps it from adding a second name, and a failed bind leaves the graph in REST; I found no new defect. Evidence: read the full diff against merge-base 4b77b7b (4 files) with GraphManager (constructor, init(), reload(), loadGraphsFromLocal(), listenChanges(), notifyEvent()), ContextGremlinServer.injectGraph(), the HugeGraphServer prepare, REST start, startPrepared() order, HugeFactory.open() (cached by graphspace and lowercased store) and HugeFactoryAuthProxy.open() (one proxy per graph); the only GRAPH_CREATE listeners are in GraphManager and ContextGremlinServer. CI run 38697372320 at this head passes; the Server memory tests log shows bind_static loading with no can't be loaded error, the name-mismatch WARN and the single failed-bind ERROR. Not reproduced against a rolling 3 PD + 3 Store cluster.

…bind-local-graphs

# Conflicts:
#	hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: Score 9/10. At f0dce63 the production change holds up. bindLocalGraph() fires GRAPH_CREATE after loadGraph() stores the graph, and the only listeners on that event are the REST map in GraphManager.listenChanges() and ContextGremlinServer.injectGraph(). Both are idempotent for the instance Gremlin already holds, because HugeFactory caches graphs by graphspace and lowercased store, and HugeFactoryAuthProxy keeps one proxy per graph. The DEFAULT-<file> guard stops a second name from being added, and a failed bind leaves the graph in REST. The merge from master touches only the UnitTestSuite registration on this PR's side. The only point here is a wording issue in the operations doc (inline). Evidence: read the full diff against merge-base a0aae8e (4 files), GraphManager (constructor and init(), which both run loadGraphsFromLocal(), so the event fires twice at startup, which is harmless; reload(), dropGraph(), dropGraphLocal(), notifyEvent()), ContextGremlinServer, HugeGremlinServer.prepare(), the HugeGraphServer order of prepare, REST start and startPrepared(), HugeFactory.open(), HugeFactoryAuthProxy.open() and StandardHugeGraph.spaceGraphName(). I also diffed 664eb9a against this head for GraphManager and the new test: the only change is the two master lines in dropGraphSpace() and PdRegisterLabel. CI run 38781033639 at this head is still running Server memory tests and Server rocksdb tests; every completed job passes. I did not run the tests locally or reproduce the rolling PD restart.

Comment thread helm/hugegraph/docs/operations.md Outdated
"Images that include apache#3228" named the bug report, which
reads as images that have the bug. Name the fixing PR instead.
@imbajin
imbajin merged commit be68aa2 into apache:master Oct 16, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] A Server that starts while PD is briefly unreachable serves REST but loses its Gremlin binding for the life of the process

3 participants