bitflicker64 commented on code in PR #814:
URL:
https://github.com/apache/hugegraph-toolchain/pull/814#discussion_r4261511700
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/entity/load/LoadTask.java:
##########
@@ -177,18 +177,34 @@ public void run() {
Ex.check(this.options != null, "The load options shouldn't be null");
log.info("LoadTask is start running : {}", this.id);
- boolean noError;
+ boolean noError = false;
+ boolean cleanupFailed = false;
+ Throwable failure = null;
try {
noError = this.loader.load();
} catch (Throwable e) {
- noError = false;
- log.error("Run task {} failed", this.id, e);
+ failure = e;
+ } finally {
+ try {
+ this.loader.shutdown();
Review Comment:
Important: with this shutdown in place, the first task of a multi-file job
to exit (finish, fail, pause or stop) takes the graph out of `LOADING` while
its siblings are still importing. `LoadTaskController.start` calls
`service.start` once per `file_mapping_ids` entry and
`LoadTaskExecutor.execute` is `@Async` on a pool with 8 core threads
(`AsyncConfig`), so the tasks of one job run concurrently against the same
graph. Each Loader sets `LOADING` in `load()` and records it only in its own
`LoadContext.loadingModeSet`; `shutdown()` then reaches `unsetLoadingMode()`,
which reads the Server mode, sees `LOADING` and writes `NONE` with no knowledge
of the other Loaders. On the Server, `GraphTransaction.prepareAdditions` runs
`checkNonnullProperty` only when the mode is not `LOADING`, so from that moment
the remaining batches of the sibling tasks are rejected for any vertex that
misses a non-null property, and whether a row is accepted depends on which task
happened to exit first. At th
e base commit Hubble never called `shutdown()` or `unsetLoadingMode()` (no
match in `hubble-be/src/main` at 8e6ed847), so every task of a job ran in
`LOADING` from start to end. Requested change: keep the mode until the last
active Loader of a graph exits, for example a per-graph count of running tasks
in `LoadTaskService` together with a way to tell `HugeGraphLoader.shutdown()`
to skip `unsetLoadingMode()` for all but the final one (today
`drainThenClose()` always calls it), and add a two-file test with staggered
completion that asserts `LOADING` is kept while either task runs and reset
after the last one.
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/service/load/LoadTaskService.java:
##########
@@ -248,17 +253,40 @@ protected void executeAfterCommit(LoadTask task) {
public void afterCommit() {
executeSafely(task);
}
+
+ @Override
+ public void afterCompletion(int status) {
+ if (status !=
TransactionSynchronization.STATUS_COMMITTED) {
+ closeUnscheduled(task, null);
+ }
+ }
});
}
private void executeSafely(LoadTask task) {
try {
this.execute(task);
} catch (RuntimeException e) {
+ closeUnscheduled(task, e);
Review Comment:
Minor: `resume` and `retry` still leave the Loader open on the same failures
this PR handles for new tasks. Both call `task.reconnect(token)`, which builds
a new `HugeGraphLoader` (clients, `TaskManager` pools and a JVM shutdown hook),
then `this.update(task)` and `this.taskExecutor.execute(...)` directly instead
of going through `executeSafely`. If the update throws, or the executor rejects
the task (queue of 1024 full), nothing calls `shutdown()` on that Loader, and
after a rejection the row stays `RUNNING` with nothing running and no entry in
`runningTaskContainer`. This is not new in this PR and the README scopes the
cleanup to new tasks, but it is the same leak on the other two dispatch paths.
Requested change: say in the PR description that these two paths are left for a
follow-up, or route them through the same close-and-mark-failed handling here.
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/entity/load/LoadTask.java:
##########
@@ -177,18 +177,34 @@ public void run() {
Ex.check(this.options != null, "The load options shouldn't be null");
log.info("LoadTask is start running : {}", this.id);
- boolean noError;
+ boolean noError = false;
+ boolean cleanupFailed = false;
+ Throwable failure = null;
try {
noError = this.loader.load();
} catch (Throwable e) {
- noError = false;
- log.error("Run task {} failed", this.id, e);
+ failure = e;
+ } finally {
+ try {
+ this.loader.shutdown();
+ } catch (Throwable e) {
+ noError = false;
+ cleanupFailed = true;
+ if (failure == null) {
+ failure = e;
+ } else if (failure != e) {
+ failure.addSuppressed(e);
+ }
+ }
+ }
+ if (failure != null) {
+ log.error("Run task {} failed", this.id, failure);
}
this.lock.lock();
try {
// Pay attention to whether the user stops actively or
// the program stops by itself
- if (this.status.inRunning()) {
+ if (cleanupFailed || this.status.inRunning()) {
Review Comment:
Minor: `cleanupFailed` is also true whenever `load()` itself threw, not only
when cleanup failed. `load()` records the error through
`context.failLoading(t)`, and `shutdown()` then rethrows it from
`TaskManager.waitFinished()` and `LoadContext.close()` via `throwIfFailed()`;
`testClosedLoaderUnregistersOwnHook` asserts exactly that rethrow. So when a
user pauses or stops a task while its load is failing, this condition now
overwrites `PAUSED` or `STOPPED` with `FAILED`, and
`LoadTaskService.pause`/`stop` persist that, where the base kept the status the
user asked for. If that is intended, a short comment and a test would pin it
down; otherwise set `cleanupFailed` only when `load()` had not already failed
(for example `cleanupFailed = failure == null` evaluated before `failure` is
assigned in this catch), keeping in mind that this also hides a real cleanup
failure, such as a shutdown timeout, that follows a failed load.
--
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]
