bitflicker64 commented on code in PR #812:
URL:
https://github.com/apache/hugegraph-toolchain/pull/812#discussion_r4250357145
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -206,10 +208,11 @@ public void updateSchemaCache() {
this.schemaCache.updateAll();
}
- public void setLoadingMode() {
+ public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
try {
this.client.graphs().mode(graph, GraphMode.LOADING);
+ this.loadingModeSet = true;
Review Comment:
Important: with the flag set only after a confirmed response, a lone Loader
that loses the response to this request now leaves the graph in `LOADING` for
good. Before this change `unsetLoadingMode()` always read the mode during
shutdown and reset `LOADING` to `NONE`, so the same failure healed itself.
Evidence: `testUnconfirmedModeSetPreservesModeAndStartupFailure` starts from
`NONE`, drops the PUT response, and asserts `server.mode == LOADING` with
`modeReads == 0` after `shutdown()`. No second Loader is involved in that case,
so nothing is being protected. The user only sees the generic I/O failure from
`load()`; nothing in the log says the mode may have been applied. On the server
a graph in `LOADING` skips `checkNonnullProperty` in `GraphTransaction`
(hugegraph-core, `if (!this.graphMode().loading())`), so later ordinary writes
are accepted without their non-null properties until someone resets the mode by
hand. A slow mode request that exceeds `--timeout` (60s default) is enough to
hit this.
Requested change: treat a failure that is not a `ServerException` as
possibly applied and set `loadingModeSet = true` before rethrowing. Cleanup
still goes through the `mode.loading()` check, a server rejection still leaves
the flag false, and the startup-failure and rejected-request cases this PR
fixes stay fixed. The response-loss test and the README paragraph would then
describe the reset. If leaving the mode in place is the intended behaviour, at
least log an error here that names the graph and says it may be left in
`LOADING` and needs a manual reset, since the README is the only place that
says so today.
--
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]
