bitflicker64 commented on code in PR #3308:
URL: https://github.com/apache/hugegraph/pull/3308#discussion_r4240715704
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -1203,26 +1204,46 @@ public void create(String configPath, GlobalMasterInfo
nodeInfo) {
this.configuration.file(confPath);
}
+ public synchronized void clearBackendForDrop() {
+ if (!this.dropBackendCleared) {
+ this.clearBackend();
+ this.dropBackendCleared = true;
+ }
+ }
+
@Override
public void drop() {
- this.clearBackend();
+ E.checkState(!this.closed() || this.dropBackendCleared,
+ "Can't drop an already closed graph '%s' before clearing
its backend",
+ this.spaceGraphName());
+ if (!this.closed()) {
+ TaskScheduler scheduler =
this.taskManager.getScheduler(this.params);
+ if (scheduler != null && !scheduler.close()) {
Review Comment:
Important: With the local scheduler (every non-hstore graph) this refuses a
drop as soon as any task is pending, where master waited up to
`task.wait_timeout` for it. `StandardTaskScheduler.close()` returns `false`
immediately when `pendingTasks() != 0` (`StandardTaskScheduler.java:449`), so
`drop()` throws here before `clearBackendForDrop()` reaches
`waitUntilAllTasksCompleted()`. On master `drop()` started with
`clearBackend()`, and that wait let short tasks finish.
I ran the same probe on both trees (memory backend, default
`task.wait_timeout=10`, one task that sleeps 1.5 s, then
`GraphManager.dropGraphLocal`):
- 87a06a9b: the drop returns after 1720 ms and the graph is closed.
- 9918ff25: the drop throws `Can't drop graph 'DEFAULT-probe_short_task'
while tasks are active, please retry later` after 0 ms. A second call after the
task ends succeeds.
So deleting a graph right after submitting an async job now fails where it
used to succeed, and admission stays closed (`admissionClosed` is never reset)
on a graph the caller may decide to keep.
Could the drop close admission first and then wait up to `task.wait_timeout`
for `pendingTasks()` to reach zero before it refuses? The wait has to sit
outside the scheduler monitor, because `finishExecution()` is `synchronized`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -564,7 +777,8 @@ public boolean close() {
}
// cancel all running tasks
- for (HugeTask<?> task : this.runningTasks.values()) {
+ for (PendingTask pending : this.runningTasks.values()) {
Review Comment:
Important: `close()` now cancels persistent tasks that were dispatched but
never started, and it does so on closes that are not deletions.
`schedule()` calls `tryStartHugeTask(task, true)` (line 306), which reserves
the task in `runningTasks` and puts it in the pool queue even when every worker
is busy. This loop then calls `cancel()` for each reservation. A reservation
that has not started owns no task lock, so line 358 writes `CANCELLING`, and
the callback at lines 365-367 saves the task as `CANCELLED`.
`checkQueuedOwnedCancellation(true)` in `DistributedTaskAdmissionTest` asserts
this outcome: the job never ran and its stored status is `CANCELLED` after
`scheduler.close()`.
On master a task submitted while the workers were busy stayed `NEW` in the
store (`tryStartHugeTask` returned `false` without submitting), `runningTasks`
held only tasks that had started, and `close()` left the backlog for cron or
another server. `GraphManager.reload()` closes graphs without deleting them, so
after a reload the accepted backlog is cancelled instead of run.
For a reservation that has not started and holds no lock, please release it
with `cancelBeforeStart()` and leave the stored status `NEW`, at least when the
close is not part of a deletion. That test would then need to expect `NEW`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -2268,40 +2266,77 @@ public void dropGraph(String graphSpace, String name,
boolean clear) {
}
String graphName = spaceGraphName(graphSpace, name);
+ boolean alreadyClosed = g.closed();
+ E.checkState(!clear || !alreadyClosed ||
this.removingGraphs.contains(graphName),
+ "Can't clear an already closed graph '%s' without an
accepted deletion", graphName);
+ // Task results still need the backend until their scheduler has
drained.
+ TaskScheduler scheduler = null;
+ if (clear && !alreadyClosed) {
+ try {
+ // Keep the auth proxy's cached owner and its pre-close ADMIN
check.
+ scheduler = g.taskScheduler();
+ } catch (IllegalStateException e) {
+ if (!(g instanceof StandardHugeGraph) ||
+ TaskManager.instance().getScheduler(g) != null) {
+ throw e;
+ }
+ // A previous plain-graph close removed its scheduler but
retained owners.
+ }
+ }
+ if (clear && scheduler != null && !scheduler.close()) {
+ throw new HugeException("Can't drop graph '%s' while tasks are
active, " +
+ "please retry later", graphName);
+ }
if (clear) {
- this.removingGraphs.add(graphName);
try {
- this.metaManager.removeGraphConfig(graphSpace, name);
+ if (!this.removingGraphs.contains(graphName)) {
+ this.metaManager.removeGraphConfig(graphSpace, name);
+ this.removingGraphs.add(graphName);
+ }
this.metaManager.notifyGraphRemove(graphSpace, name);
} catch (Exception e) {
throw new HugeException(
- "Failed to remove graph config of '%s'", name, e);
+ "Failed to remove graph config of '%s'", e, name);
}
- /**
- * close task scheduler before clear data,
- * because taskinfo stored in backend in
- * {@link org.apache.hugegraph.task.DistributedTaskScheduler}
- */
+ if (!alreadyClosed) {
+ // Unwrapping an auth proxy preserves its ADMIN permission
check.
+ HugeGraph underlying = g.hugegraph();
+ if (underlying instanceof StandardHugeGraph) {
+ ((StandardHugeGraph) underlying).clearBackendForDrop();
+ } else {
+ g.clearBackend();
+ }
+ }
try {
- g.taskScheduler().close();
- } catch (Throwable t) {
- LOG.warn(String.format(
- "Error when close TaskScheduler of %s",
- graphName),
- t);
+ if (!alreadyClosed) {
+ g.close();
+ }
+ } catch (Exception e) {
+ if (!g.closed()) {
+ throw new HugeException("Failed to close graph '%s',
please retry later",
+ e, graphName);
+ }
+ LOG.warn("Graph '{}' closed with an owner cleanup failure",
graphName, e);
}
-
- g.clearBackend();
+ } else {
+ // Remote notifications detach owners only after close succeeds.
try {
g.close();
} catch (Exception e) {
- LOG.warn("Failed to close graph", e);
+ if (!g.closed()) {
+ throw new HugeException("Failed to close graph '%s',
please retry later",
Review Comment:
Important: This throw also runs under `reload()` (line 704), which calls
`dropGraph(space, name, false)` for every graph in a loop with no catch.
On master the non-clear path had no close check: a close failure was logged
after `this.graphs.remove`, the graph was detached anyway, and `reload()` went
on to `loadGraphsFromMeta()`. At this head the first graph whose close is
incomplete aborts `reload()`. On this PD-only path that means
`DistributedTaskScheduler.close()` returned `false` (a cancelled task still
running after the 10 s wait at line 791, or the cron barrier timing out at line
764), or a non-hstore graph with one pending task. Graphs iterated before that
one are already removed from `this.graphs` and are not reopened, because the
load steps after the loop are skipped. They stay unavailable on this server
until a later reload gets through.
`graphRemoveHandler` catches the exception at line 2728, `reload()` does
not. Please catch per graph in `reload()` and still run the load steps for the
graphs that were detached before reporting the ones that could not close. The
`this.graphs.isEmpty()` check at lines 707-712 needs the same treatment, since
a retained graph would otherwise skip the load steps as well.
--
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]
