Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3329 +/- ##
============================================
+ Coverage 54.31% 57.09% +2.78%
+ Complexity 2250 1409 -841
============================================
Files 770 588 -182
Lines 67952 56420 -11532
Branches 9213 7930 -1283
============================================
- Hits 36905 32215 -4690
+ Misses 27470 21025 -6445
+ Partials 3577 3180 -397 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- register late graph services before publication - serialize cold startup and graph publication - remove exact graph-owned RPC resources - preserve legacy cleanup and verify lifecycle
- enforce the existing ADMIN gate before unregistering - preserve RPC owners after denied Gremlin cleanup - isolate event and DB executors in graph tests - cover null contexts and suite shutdown ordering
- scope trusted lazy graph RPC registration - serialize graph export and cleanup by service - retain graph owners when pre-delete cleanup fails - document cleanup and retry boundaries
- wait for actual listener closure before fixture port reuse - retain production RPC and graph cleanup behavior - verify delayed close followed by same-port RPC calls
- retain callback transaction cleanup from original merge - preserve failed graph rollback owner regression tests - restore documented RPC deletion and cleanup guarantees
1203616 to
e7fb041
Compare
register rpc before publishing local graph events restore retained graph rpc after metadata failure preserve primary errors and clean partial rpc resources cover production listener order and deletion failure stages
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Late and lazily loaded graphs now register and export their cache notifier services before they are published, and the create, rollback and restore paths hold up against the code at this head. The rewritten close handling in dropGraph regresses two cases master tolerated: a close failure of another type aborts the rest of a removal batch on peers, and a graph that closed with an error is left registered with no working retry. Evidence: full diff at 11a3cfc (15 files, +1532/-112) read against master 0b7c0de; GraphManager.dropGraph, graphRemoveHandler, graphAddHandler, publishGraph, rollbackGraph, startRpcServer, StandardHugeGraph.close, registerRpcServices, taskScheduler, clearBackend, TaskManager.closeScheduler, RpcConsumerConfig, RpcProviderConfig, RpcServer; sofa-rpc-all 5.7.6 sources for ProviderConfig.setServer, DefaultProviderBootstrap.doExport/unExport, DefaultConsumerBootstrap.unRefer and BoltServer; GraphManagerConfigTest is in UnitTestSuite; all required CI jobs passed at this head (codecov/project is the only failure). I did not run the server unit suite. Cross-server invalidation against a real PD and shared backend was not exercised.
- retain graph ownership until native cleanup completes - drain context executors without closing shared listeners - reject uncleared closed owners and preserve fatal errors - deduplicate RPC exports and verify native retry paths
imbajin
left a comment
There was a problem hiding this comment.
P2 follow-up — Preserve the local deletion retry path when resource cleanup fails
Reviewed commit: cf26235.
The new cleanup queue in StandardHugeGraph.close() (around lines 1215–1240) retains failed operations, including RPC unregistration, and correctly allows a later close() to retry. The local non-PD DELETE path still calls drop(), whose existing catch (around lines 1283–1292) suppresses close failures once closed() is true, then deletes the configuration. GraphManager.dropGraphLocal() subsequently removes the factory binding.
If RPC unregistration keeps failing, the lenient GRAPH_DROP callback can leave only a closed manager entry behind, while the configuration and factory identity have already been removed. Retrying DELETE enters drop() and fails checkGraphNotClosed() rather than draining the retained cleanup queue. For a provider/native cleanup failure, the remaining manager entry may also be removed after successful unpublication.
The swallowing catch and generic cleanup limitation already exist on the base, so this is a remaining local lifecycle gap, not a claim that the PR newly introduced generic close-error suppression. The new RPC cleanup/retry contract makes it worth covering explicitly. Could we retain the local deletion identity until cleanup completes and add a non-PD persistent-unregister-failure → retry regression, or document that this local error path requires recovery outside DELETE? The new fault-injection cases currently exercise the PD path; the local deletion case covers successful cleanup.
Evidence boundary: independently cross-checked the exact-head source, caller chain, tests, and base implementation. No live failure injection was executed in this review.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9/10 (10 - 0.5 Minor duplicated close-failure helper - 0.5 Minor already raised local DELETE retry gap = 9). Summary: Late graphs now register and export their cache notifier services before they become visible, cleanup retries stay on the same owner, and the earlier review points (busy-graph batch abort, closed-owner retry, duplicate server bindings on exportAll, local publish ordering) are addressed at this head. Evidence: read the full diff at cf26235 and the callers in GraphManager, StandardHugeGraph, HugeFactory, TaskManager, RaftContext/RaftNode and the sofa-rpc 5.7.6 / jraft 1.3.9 bytecode for unRefer, AbstractCluster.destroy and RaftGroupService.shutdown; ran ServerClientTest and RpcConfigTest (31 tests, pass) and GraphManagerConfigTest with -P unit-test (42 tests, pass); a probe of RpcConsumerConfig.serviceProxy against an unreachable peer returned in about 1 s, so refer under the GraphManager monitor does not wait for the 20 s connect timeout; all required CI checks pass at this head. Cross-server invalidation against a real PD and shared backend was not exercised.
Already raised: @imbajin (review at cf26235) on the local non-PD DELETE path, where a persistent RPC unregistration failure leaves a closed manager entry that a later DELETE cannot drain.
| return this.tx; | ||
| } | ||
|
|
||
| private static Throwable collectCloseFailure(Throwable first, Throwable next) { |
There was a problem hiding this comment.
Minor: collectCloseFailure and referencesFailure are copied verbatim into three classes: here, RaftBackendStoreProvider (line 195) and RaftContext (line 220).
The three copies implement the same Error-priority and cycle-avoidance rules. A later fix to one copy (for example how a cycle is logged) will not reach the other two, and the close paths would then report failures differently.
Move the two methods into one public helper in hugegraph-core (for example next to LockUtil in org.apache.hugegraph.util) and call it from all three close paths.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Late, lazily loaded and PD-added graphs now register and export their cache notifiers before they become visible, and the originating server's clearing DELETE can retry a failed close through the clear certificate. On a PD peer, a close that fails after closed = true now leaves the closed graph published with no retry path. A later graph created under the same name is then never loaded on that peer until a full reload or a restart. Evidence: read the full diff at cf26235 (18 files, +3074/-178) against merge base 92af2f8. Traced GraphManager.dropGraph, graphRemoveHandler, graphAddHandler, reload, graph(String, String), publishGraph, unpublishGraph, rollbackGraph, startRpcServer, GraphsAPI.drop, StandardHugeGraph.close/registerRpcServices/unregisterRpcServices, HugeFactory.open/remove, RaftContext.close, RaftNode.shutdown, RpcConsumerConfig and RpcProviderConfig. Compared master's dropGraph, which logged a close failure on a closed graph and removed the binding. Checked the retry fixtures in GraphManagerConfigTest (around lines 870-944): they retry by calling dropGraph(..., false) directly. All required CI checks pass at this head. I did not run tests: library was unreachable, and the finding follows from static control flow. The collectCloseFailure duplication (open thread) and the non-PD local DELETE retry gap (imbajin's review) are already raised and not repeated here.
| g.close(); | ||
| } catch (Exception failure) { | ||
| // Logical closed does not certify native release; keep this owner until close succeeds. | ||
| throw new HugeException("Failed to close graph '%s', please retry later", failure, graphName); |
There was a problem hiding this comment.
Important: on a PD peer, this rethrow leaves a closed graph published with no way to retry, and the peer then skips any graph created later under the same name.
The peer path is graphRemoveHandler -> dropGraph(gs, name, false) on a grpc thread. Suppose g.close() fails after closed = true, for example in the new unregisterRpcServices step, storeProvider.close() or the Raft executor drain. The HugeException thrown here is caught and only logged in graphRemoveHandler (line 2808). g stays in graphs, in HugeFactory and in the Gremlin bindings, and GRAPH_DROP is never fired.
Nothing on that peer retries the close:
- the PD remove event has already been consumed;
- a REST DELETE on the peer passes
clear=true(GraphsAPI.drop), and line 2357 refuses a closed owner that has no clear certificate; - when the graph is created again under the same name,
graphAddHandlerseesthis.graphs.containsKey(graphName)(line 2749) and skips it, so requests to this peer keep resolving to the closed instance.
Only PUT .../graphs/manage with action=reload (closes and reloads every graph on the server) or a restart recovers. On master the same failure on a closed graph was logged, and the binding and factory entry were removed, so a re-created graph loaded normally.
The new fault-injection tests (for example around lines 911-944) retry by calling manager.dropGraph("DEFAULT", name, false) again. No production caller does that for a peer.
Requested change: give the peer path a real retry trigger. One option: when graphAddHandler (or graph(graphSpace, name)) finds an existing entry that is closed(), run dropGraph(parts[0], parts[1], false) again before deciding to skip, and load the new config once that succeeds. Another option: queue failed peer removals for a background retry. Please add a test that drives graphRemoveHandler with a failing close, then graphAddHandler for the same name, and asserts that the replacement is loaded once cleanup succeeds.
Purpose of the PR
Graphs created after startup could miss RPC cache invalidation registration, leaving other Server instances with stale cached data. Export each graph before publishing it and retain its ownership until cleanup actually completes.
Main Changes
Verifying these changes
Current-head CI remains a separate gate on both repositories. The native shared-listener close fixture controls a separate duplicate-processor startup defect; production multi-graph startup, broader PD/shared-backend cross-Server DML and complete HTTP authentication remain release validation tasks.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedRuntime and retry boundaries: docs/server-runtime.md.
Conceptual PD-enabled lifecycle; the runtime document defines the local-deletion, failure and retry boundaries.
Paired source PR: hugegraph#303