bitflicker64 commented on code in PR #3329:
URL: https://github.com/apache/hugegraph/pull/3329#discussion_r4268028116
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -2274,53 +2376,70 @@ public void dropGraph(String graphSpace, String name,
boolean clear) {
"Retry deletion after owners exit, or restart to
resume task admission",
graphName);
}
+ // Cleanup failure must precede metadata removal and destructive
clear.
+ this.unregisterRpcServices(g);
this.removingGraphs.add(graphName);
try {
this.metaManager.removeGraphConfig(graphSpace, name);
this.metaManager.notifyGraphRemove(graphSpace, name);
} catch (Exception e) {
+ // No backend clear has started; retain RPC for the same open
local owner.
+ synchronized (this) {
+ if (this.graphs.get(graphName) == g && !g.closed()) {
+ try {
+ this.registerRpcServices(g);
+ } catch (RuntimeException | Error restoration) {
+ if (e != restoration) {
+ e.addSuppressed(restoration);
+ }
+ }
+ }
+ }
throw new HugeException(
- "Failed to remove graph config of '%s'", name, e);
+ "Failed to remove graph config of '%s'", e, name);
}
g.clearBackend();
- try {
- g.close();
- } catch (Exception e) {
- if (!g.closed()) {
- throw new HugeException("Failed to close graph '%s',
please retry later", e, graphName);
+ synchronized (this) {
+ if (this.graphs.get(graphName) == g) {
+ this.clearedGraphs.put(graphName, g);
}
- LOG.warn("Failed to close graph", e);
}
}
- GraphSpace gs = this.graphSpace(graphSpace);
- if (!grpcThread) {
- gs.recycleGraph();
- LOG.info("The graph_number_used successfully decreased to {} " +
- "of graph space: {} for graph: {}",
- gs.graphNumberUsed(), gs.name(), name);
- this.metaManager.updateGraphSpaceConfig(graphSpace, gs);
- }
- // Let gremlin server and rest server context remove graph
- LOG.info("Notify remove graph {} by GRAPH_DROP event", name);
- Graph graph = this.graphs.get(graphName);
- if (graph != null) {
- try {
- graph.close();
- } catch (Exception e) {
- if (!((HugeGraph) graph).closed()) {
- throw new HugeException("Failed to close graph '%s',
please retry later", e, graphName);
+ try {
+ 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);
Review 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, `graphAddHandler`
sees `this.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.
--
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]
