bitflicker64 commented on code in PR #3308:
URL: https://github.com/apache/hugegraph/pull/3308#discussion_r4253638599
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -1212,12 +1213,16 @@ public void create(String configPath, GlobalMasterInfo
nodeInfo) {
@Override
public void drop() {
+ this.checkGraphNotClosed();
+ TaskScheduler scheduler = this.taskManager.getScheduler(this.params);
+ if (scheduler != null && !scheduler.close()) {
+ // Fence admission before waiting for actual task owners to exit.
+ this.waitUntilAllTasksCompleted();
Review Comment:
Important: When the drop is refused here, the graph is kept but its local
scheduler never accepts work again. `scheduler.close()` on line 1218 sets
`admissionClosed = true` (`StandardTaskScheduler.java:444`), that is the only
assignment to the field, and `schedule()` and `restoreTasks()` then throw `Task
scheduler for graph '...' is closing`.
`StandardTaskAdmissionTest.testCancelledWorkerFencesAdmissionUntilActualExit`
asserts this after a `close()` that returned `false`.
On master a drop that timed out in `clearBackend()` had not touched the
scheduler, so the graph stayed fully usable. At this head, with the default
`task.wait_timeout=10`, a delete issued while a job longer than 10 s is running
is refused, and from then on every async job on that graph (index rebuild,
schema removal, Gremlin job) fails until a later delete succeeds or the server
restarts. The new section in `docs/server-runtime.md` says the graph stays
registered with its data and configuration and does not mention this. The fence
in `GraphManager.dropGraph` (lines 2264-2272) has the same effect for a graph
on the local scheduler.
Please reopen admission when the drop is refused, in both places. If the
fence is meant to stay closed, the error message and the runtime doc should say
that the graph accepts no tasks until it is deleted or the server restarts.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskManager.java:
##########
@@ -261,7 +251,8 @@ public ServerInfoManager
getServerInfoManager(HugeGraphParams graph) {
}
public void shutdown(long timeout) {
- assert this.schedulers.isEmpty() : this.schedulers.size();
+ E.checkState(this.schedulers.isEmpty(),
Review Comment:
Important: `HugeFactory.shutdown()` now fails, and leaves every task
executor running, whenever one scheduler is still registered. On master this
line was an `assert`, which the server does not enable
(`bin/hugegraph-server.sh` passes no `-ea`), so shutdown went on to
`shutdown()` and `awaitTermination(timeout)` each executor.
A scheduler stays registered in ordinary cases, not only after a refused
deletion. `StandardTaskScheduler.close()` returns `false` as soon as
`pendingTasks() != 0` (line 447), and `closeScheduler()` then keeps it (line
132). Stopping a server while one async task is running therefore goes:
`graph.close()` fails, `HugeGraphServer.stop()` calls
`HugeFactory.shutdown(30L)`, this check throws, and `stop()` only logs `Failed
to stop HugeGraph`. The running task loses the up to 30 s it had on master to
finish and save its status, `OltpTraverser.destroy()` is skipped, and an
embedded caller of `HugeFactory.shutdown(timeout)` now gets `HugeException:
Failed to shutdown` with the executors still alive.
This is from reading the code at bb3ebc85 against 68fb0453, not from a run.
Could `shutdown()` still stop the executors when schedulers remain? For
example: retry `close()` on the retained schedulers within `timeout`, shut the
executors down either way, and name the schedulers that never drained in the
exception thrown at the end.
--
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]
