bitflicker64 commented on code in PR #814:
URL:
https://github.com/apache/hugegraph-toolchain/pull/814#discussion_r4266618501
##########
hugegraph-hubble/hubble-be/src/main/java/org/apache/hugegraph/entity/load/LoadTask.java:
##########
@@ -177,18 +178,57 @@ 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 {
+ Throwable recordedFailure = failure;
+ if (failure != null) {
+ // Only read the recorded error: shutdown can wrap a checked
+ // failure differently from load(), or rethrow it unchanged.
+ try {
+ this.context().throwIfFailed();
+ } catch (Throwable recorded) {
+ recordedFailure = recorded;
+ if (recorded != failure) {
+ // Retain an asynchronously recorded error even when it
+ // differs from the error propagated by load().
+ log.error("Recorded load failure for task {}",
this.id, recorded);
+ }
+ }
+ }
+ Throwable recordedCause = recordedFailure == null ? null :
recordedFailure.getCause();
+ int suppressed = recordedFailure == null ? 0 :
recordedFailure.getSuppressed().length;
+ int causeSuppressed = recordedCause == null ? 0 :
recordedCause.getSuppressed().length;
+ try {
+ this.loader.shutdown();
+ } catch (Throwable e) {
+ noError = false;
+ cleanupFailed = !sameLoadFailure(recordedFailure, e) ||
Review Comment:
Important: a failed mode reset is still lost when the load itself failed.
`HugeGraphLoader.drainThenClose()` runs `try { this.context.unsetLoadingMode();
} finally { this.context.close(this.loadStarted); }`. With a recorded load
failure, `close()` throws it again from `throwIfFailed()` (for a checked one, a
new `LoadException` around the same cause), and that replaces the
`LoadException("Failed to unset mode LOADING for server")` from the `try`,
which is neither logged nor attached as suppressed. `sameLoadFailure()` then
returns true and `cleanupFailed` stays false. A throwaway test at 2b2d66c6
confirms it: against an HTTP stub that accepts the `LOADING` `PUT` and then
fails the mode `GET`, with `load()` stubbed to set the mode, record a failure
and throw, a paused or stopped task ends `PAUSED` or `STOPPED`, the stub never
receives a `PUT` of `NONE`, and the only error logged is `Run task 9 failed`.
By the same path a task that was not paused ends `FAILED` from the load error,
als
o without the `check graph loading mode` line. So the README sentence `An
independent shutdown failure marks the task as failed; check the logs and graph
loading mode before retrying` does not hold when the load also failed. Not
blocking: this `try`/`finally` is unchanged from the base, where Hubble never
reset the mode, so the end state is the same. Requested change: in
`drainThenClose()`, catch the `unsetLoadingMode()` failure and add it as
suppressed to whatever `close()` throws (rethrow it when `close()` returns
normally). That makes this check true for a runtime failure through the
suppressed count on the next line, and for a checked one through the
`getSuppressed().length == 0` clause of `sameLoadFailure()`. Please add a
Loader test with a recorded failure plus a failing mode reset.
--
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]
