You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "imbajin (via GitHub)" <gi...@apache.org> on 2026/10/13 05:18:56 UTC
[PR] fix(loader): preserve loading ownership [hugegraph-toolchain]
imbajin opened a new pull request, #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815
Source PR: https://github.com/hugegraph/hugegraph-toolchain/pull/68.
## Purpose of the PR
When two imports share a graph, the first import finishing can reset its mode while the other still writes. Give each ordinary import its own loading handle so the graph stays in LOADING until all owners stop and release it.
This is stacked on the [Hubble cleanup change](https://github.com/apache/hugegraph-toolchain/pull/814) and requires the paired [Server ownership API](https://github.com/apache/hugegraph/pull/3331).
## Main Changes
- Add Client capability and ownership calls, and retain one fixed handle per Loader attempt, including uncertain acquire outcomes.
- Release only after workers drain. Preserve RESTORING cleanup after its successful mode change, and reject unsupported Servers before clearing data or creating schema.
- Use the paired protocol-1 Server for Loader, embedded Hubble imports and the Loader image smoke. Official Server 1.7 instead verifies refusal with unchanged graph state; Hubble keeps its other API and UI checks.
## Verifying these changes
- [x] Need tests and can be verified as follows:
- Client, Loader and Hubble unit tests; formatting, compilation and CI helper checks.
- Actual concurrent Loader imports and native data readback for the unchanged ordinary-owner path.
- Actual official Server 1.7 refusal controls for ordinary and RESTORING imports, including shutdown and unchanged schema, data and mode.
The combined Hubble two-file/browser run, Docker end-to-end tests, real PD/shared-backend validation and new workflow executions remain pending.
## Does this PR potentially affect the following parts?
- [ ] Nope
- [ ] Dependencies (add/update license info)
- [x] Modify configurations
- [x] The public API
- [x] Other affects: import compatibility and graph mode cleanup
## Documentation Status
- [ ] `Doc - TODO`
- [x] `Doc - Done`
- [ ] `Doc - No Need`
Loader README and CI documentation are updated here. Website guidance is paired with [apache/hugegraph-doc#525](https://github.com/apache/hugegraph-doc/pull/525); coordinate its merge with the Server and Client/Loader changes.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4275948901
##########
hugegraph-loader/README.md:
##########
@@ -145,14 +147,86 @@ failure and logs an error naming the graph and warning that it may still be in `
unconfirmed mode unchanged. Inspect the graph mode and confirm there are no active imports before explicitly
restoring `NONE` mode; an unconfirmed request cannot be rolled back automatically.
+#### Concurrent imports and graph mode
+
+On a Server with loading-owner protocol 1, each ordinary import acquires its own
+handle on the same live graph instance. After its workers stop, cleanup releases
+only that handle; the graph stays in `LOADING` until the last import releases.
+The protocol does not coordinate different Server processes or survive restart.
+
+Servers without the ownership endpoint keep the existing LOADING and cleanup
+behavior, with no concurrent-import guarantee. Permission errors, network failures
+and invalid ownership responses do not fall back to legacy mode. Restore and
+Graph-source imports retain their existing RESTORING behavior.
+
+The paired Server change must be merged and released before the new guarantee is
+available: [Server #3331](https://github.com/apache/hugegraph/pull/3331).
+
+On protocol-1 Servers, an ordinary import requires `NONE` or `LOADING` held by
+active owners. Restore and Graph-source imports leave `RESTORING` after shutdown,
+as before. A subsequent ordinary import is rejected until that mode is manually
+recovered; an enum-set `LOADING` or `MERGING` mode has the same prerequisite.
+
+Before acquiring ownership, Loader logs `Preparing LOADING owner` with the
+non-secret `instance/epoch` identity and recovery guidance. Owner tokens are not
+logged. Owners have no automatic expiry, and cleanup failure still closes the
+Loader context and clients; another `shutdown()` does not retry the release.
+After an unclean exit or release failure, use the paired Server's administrator
+recovery procedure below instead of looking for a token in Loader logs.
+
+Hubble's existing task lifecycle can finish, stop or reconnect without calling
+`Loader.shutdown()`. This SDK change alone can therefore leave active owners and
+block mode changes, including restore. Hubble integration requires the paired
+[task cleanup fix #814](https://github.com/apache/hugegraph-toolchain/pull/814) and
+[application shutdown fix #818](https://github.com/apache/hugegraph-toolchain/pull/818);
+deploy those fixes together. If owners are already stranded, use the recovery
+steps below.
+
+1. Stop all related imports and ordinary graph writes, and drain their in-flight
+ requests. The Server recovery endpoint cannot perform this external drain.
+2. As an administrator with `ADMIN` or `ROOT` permission, GET
+ `/graphspaces/{graphspace}/graphs/{graph}/mode/owners` on the affected live
+ Server instance. Read its current `instance`, `epoch` and non-secret `revision`
+ immediately before recovery; do not reuse values from another instance or an
+ earlier attempt.
+3. PUT the following body to that same endpoint, replacing the placeholders with
+ the values just read. This recovery action does not require an owner token.
+
+ ```json
+ {
+ "action": "recover",
+ "instance": "<current instance UUID>",
+ "epoch": 0,
+ "revision": 0,
+ "confirm": "I'm sure all imports have stopped"
+ }
+ ```
+
+ Replace both example `0` values with the GET response's current `epoch` and
+ `revision`. Recovery requires both values to match; any owner or mode change
+ after GET makes this request stale.
+ Success returns `state: recovered`, `mode: NONE` and the incremented epoch,
+ clears existing owners, and prevents old-epoch acquire/release requests from
+ taking effect. A stale recovery request returns HTTP 409 so it cannot clear a
+ newer owner, including one acquired within the same epoch. Ordinary graph-status
+ write permission is insufficient (HTTP 403).
+4. If the response is lost or the result is otherwise unknown, inspect the graph
+ mode and capability. Do not blindly obtain a new epoch and repeat recovery,
+ which could clear owners created after the first attempt.
+
+This procedure also clears a manually set `RESTORING` or `MERGING` mode once all
+related work has stopped and drained. It depends on the paired Server change
+linked above; older Servers retain their legacy enum-mode recovery behavior.
+An enum PUT to `NONE` remains rejected while protocol-1 owners are active.
Review Comment:
Minor: this line says only an enum PUT to `NONE` is rejected while owners are active, but line 179 says owners block mode changes generally, and the paired Server agrees with line 179: at apache/hugegraph#3331 (33887ab) `LoadingMode.mode(GraphMode)` rejects every enum PUT with HTTP 409 while any owner is active, and its `docs/server-runtime.md` says so.
That also affects clients still on the enum path. `HGVertexWriter`/`HGEdgeWriter` call `HGLoadContext.setLoadingMode()` in their constructors, which PUTs `LOADING`; the 409 message does not contain `Can not deserialize`, so every Spark writer task fails while a protocol-1 Loader import runs. A Loader from before this change fails at startup, and a concurrent `--restore` fails on its `RESTORING` PUT.
Could this line say that any enum mode change is rejected, naming the Spark connector and older Loaders?
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4264342440
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,27 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
+ } catch (ServerException e) {
+ if (e.status() != 404 || !"HTTP 404 Not Found".equals(e.getMessage()) ||
+ !("jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "javax.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class javax.ws.rs.NotFoundException".equals(e.exception()))) {
+ throw e;
+ }
+ LOG.warn("Server has no loading-owner endpoint; use legacy mode without concurrent-import protection");
+ }
+ }
+ if (this.loadingOwner != null) {
+ this.client.graphs().acquireLoadingOwner(graph, this.loadingOwner);
Review Comment:
The README now explicitly requires NONE or owner-held LOADING for ordinary imports on a protocol-1 Server. It documents stopping all imports/writes and draining requests before administrator recovery using the current instance, epoch, and revision; it does not automatically overwrite RESTORING. A Client/LoadContext rejection regression verifies there is no enum fallback. The real paired Server/Loader run verifies restore leaves RESTORING, controlled recovery restores NONE, and a subsequent ordinary import succeeds. Real Server 1.7 ordinary/restore behavior remains unchanged.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,27 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
Review Comment:
We addressed lost-handle recovery through the paired privileged Server recovery endpoint rather than publishing the owner token. Pre-acquire diagnostics record the non-secret instance and epoch, and LoadingModeOwner.toString now omits the token. The README describes exact instance/epoch/revision recovery after an administrator stops and drains all imports and writes; no original token is required. The real paired run verifies this recovery and rejects late reuse of the old Loader handle without an automatic new-token or enum fallback.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4267203044
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,29 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
+ LOG.info("Preparing LOADING owner {} for graph '{}'; after an unclean exit, stop related imports " +
+ "and use the Server loading-owner recovery procedure", this.loadingOwner, graph);
+ } catch (ServerException e) {
+ if (e.status() != 404 || !"HTTP 404 Not Found".equals(e.getMessage()) ||
+ !("jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "javax.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class javax.ws.rs.NotFoundException".equals(e.exception()))) {
+ throw e;
+ }
+ LOG.warn("Server has no loading-owner endpoint; use legacy mode without concurrent-import protection");
+ }
+ }
+ if (this.loadingOwner != null) {
+ this.client.graphs().acquireLoadingOwner(graph, this.loadingOwner);
Review Comment:
Minor: Hubble never releases this handle. `LoadTask.run()` calls only `loader.load()`, and `stop()` and `reconnect()` drop the loader without `shutdown()`, so `unsetLoadingMode()` runs only from the JVM shutdown hook.
On legacy Servers Hubble already left LOADING behind, but an enum PUT to NONE cleared it. On a #3331 Server each Hubble task and each resume adds an active owner. The Server then rejects every enum mode change, including restore, until Hubble exits or an administrator runs `recover`.
Could Hubble call `loader.shutdown()` at the end of `LoadTask.run()` and in `stop()`, or could the README call out this Hubble limitation next to the recovery steps?
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -242,6 +269,18 @@ public void setRestoreMode() {
}
public synchronized void unsetLoadingMode() {
+ if (this.loadingOwner != null) {
+ if (!this.loadingOwnerReleased) {
+ try {
+ this.client.graphs().releaseLoadingOwner(this.client.graph().graph(), this.loadingOwner);
+ this.loadingOwnerReleased = true;
+ } catch (Exception e) {
+ throw new LoadException("Failed to release LOADING owner %s; retry the same handle", e,
Review Comment:
Minor: the release-failure message tells the user to "retry the same handle", but nothing at this head can do that.
`stopThenShutdown()` calls `context.close()` right after this throws, which closes the clients and sets `closed`, so a second `shutdown()` returns early. `LoadingModeOwner.toString()` no longer includes the token, so this message and the `Preparing LOADING owner` log give only `instance/epoch`. README line 198 says the opposite: another `shutdown()` does not retry, so use administrator recovery.
Point the message at the Server recovery procedure from the README instead of a retry.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4274194162
##########
hugegraph-loader/README.md:
##########
@@ -171,3 +171,67 @@ Server 1.7 compatibility CI uses the pinned official ASF 1.7.0 binary archive. T
hugegraph-loader is licensed under Apache 2.0 License.
See the [Flink CDC Java 17 validation](docs/flink-cdc-java17.md) for its current limits and launcher syntax.
+
+### Concurrent imports and graph mode
Review Comment:
Moved the ownership and administrator recovery section into Usage 3.1, directly after graph-mode startup and cleanup, and added an anchor link from that subsection. The Flink CDC link is now under Doc; License contains only the license text. Local paths, the anchor, section placement and whitespace checks pass.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,29 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
+ LOG.info("Preparing LOADING owner {} for graph '{}'; after an unclean exit, stop related imports " +
+ "and use the Server loading-owner recovery procedure", this.loadingOwner, graph);
+ } catch (ServerException e) {
+ if (e.status() != 404 || !"HTTP 404 Not Found".equals(e.getMessage()) ||
+ !("jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "javax.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class javax.ws.rs.NotFoundException".equals(e.exception()))) {
+ throw e;
+ }
+ LOG.warn("Server has no loading-owner endpoint; use legacy mode without concurrent-import protection");
+ }
+ }
+ if (this.loadingOwner != null) {
+ this.client.graphs().acquireLoadingOwner(graph, this.loadingOwner);
Review Comment:
Added the requested README warning immediately before administrator recovery: this SDK change alone can leave Hubble owners active after completion, stop or reconnect, blocking mode changes including restore. It explicitly requires deploying the paired task cleanup #814 and application shutdown #818 changes together. Those fixes remain separate PRs; this documentation does not claim they have landed or passed final combined acceptance.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4267307926
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -242,6 +269,18 @@ public void setRestoreMode() {
}
public synchronized void unsetLoadingMode() {
+ if (this.loadingOwner != null) {
+ if (!this.loadingOwnerReleased) {
+ try {
+ this.client.graphs().releaseLoadingOwner(this.client.graph().graph(), this.loadingOwner);
+ this.loadingOwnerReleased = true;
+ } catch (Exception e) {
+ throw new LoadException("Failed to release LOADING owner %s; retry the same handle", e,
Review Comment:
Updated the diagnostic to direct users to stop and drain related imports, then use the Server administrator recovery procedure documented in the README. It no longer suggests retrying a handle after the context and clients have closed. The owner protocol and cleanup behavior are unchanged. Formatting, the affected Client/Loader compilation and their existing unit suites passed.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): preserve loading ownership [hugegraph-toolchain]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#issuecomment-6138941588
## [Codecov](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `87.95181%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.93%. Comparing base ([`8e6ed84`](https://app.codecov.io/gh/apache/hugegraph-toolchain/commit/8e6ed84777387a586f13d713568b11a654d8d1fa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de942c5`](https://app.codecov.io/gh/apache/hugegraph-toolchain/commit/de942c5ebe9fd2bbd5b8776a3534b57aed6448bb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/hugegraph/loader/HugeGraphLoader.java](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&filepath=hugegraph-loader%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Floader%2FHugeGraphLoader.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLWxvYWRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2xvYWRlci9IdWdlR3JhcGhMb2FkZXIuamF2YQ==) | 85.00% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/hugegraph/loader/progress/InputItemProgress.java](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&filepath=hugegraph-loader%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Floader%2Fprogress%2FInputItemProgress.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLWxvYWRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2xvYWRlci9wcm9ncmVzcy9JbnB1dEl0ZW1Qcm9ncmVzcy5qYXZh) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/hugegraph/loader/progress/InputProgress.java](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&filepath=hugegraph-loader%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Floader%2Fprogress%2FInputProgress.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLWxvYWRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2xvYWRlci9wcm9ncmVzcy9JbnB1dFByb2dyZXNzLmphdmE=) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/hugegraph/loader/reader/file/FileReader.java](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&filepath=hugegraph-loader%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Floader%2Freader%2Ffile%2FFileReader.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLWxvYWRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2xvYWRlci9yZWFkZXIvZmlsZS9GaWxlUmVhZGVyLmphdmE=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #815 +/- ##
=============================================
+ Coverage 47.04% 57.93% +10.89%
+ Complexity 4517 1204 -3313
=============================================
Files 602 111 -491
Lines 29448 6305 -23143
Branches 3454 851 -2603
=============================================
- Hits 13853 3653 -10200
+ Misses 14312 2384 -11928
+ Partials 1283 268 -1015
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hugegraph-toolchain/pull/815?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#issuecomment-6149710401
Addressed [the release-error review](https://github.com/apache/hugegraph-toolchain/pull/815#pullrequestreview-5505204980). When release and context cleanup both fail, shutdown now preserves the original release failure and its instance/epoch/token recovery handle, with the cleanup failure attached as suppressed. Resources still close, and a cleanup-only failure keeps its original exception.
Regression checks cover lost replies and real logger cleanup. The published two-file update matches the source checked by all three independent reviewers. Recovery remains an explicit REST release with the same handle and authenticated identity. Current-head CI and full matching Server/Loader/Hubble verification remain open.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4276231500
##########
hugegraph-loader/README.md:
##########
@@ -145,14 +147,86 @@ failure and logs an error naming the graph and warning that it may still be in `
unconfirmed mode unchanged. Inspect the graph mode and confirm there are no active imports before explicitly
restoring `NONE` mode; an unconfirmed request cannot be rolled back automatically.
+#### Concurrent imports and graph mode
+
+On a Server with loading-owner protocol 1, each ordinary import acquires its own
+handle on the same live graph instance. After its workers stop, cleanup releases
+only that handle; the graph stays in `LOADING` until the last import releases.
+The protocol does not coordinate different Server processes or survive restart.
+
+Servers without the ownership endpoint keep the existing LOADING and cleanup
+behavior, with no concurrent-import guarantee. Permission errors, network failures
+and invalid ownership responses do not fall back to legacy mode. Restore and
+Graph-source imports retain their existing RESTORING behavior.
+
+The paired Server change must be merged and released before the new guarantee is
+available: [Server #3331](https://github.com/apache/hugegraph/pull/3331).
+
+On protocol-1 Servers, an ordinary import requires `NONE` or `LOADING` held by
+active owners. Restore and Graph-source imports leave `RESTORING` after shutdown,
+as before. A subsequent ordinary import is rejected until that mode is manually
+recovered; an enum-set `LOADING` or `MERGING` mode has the same prerequisite.
+
+Before acquiring ownership, Loader logs `Preparing LOADING owner` with the
+non-secret `instance/epoch` identity and recovery guidance. Owner tokens are not
+logged. Owners have no automatic expiry, and cleanup failure still closes the
+Loader context and clients; another `shutdown()` does not retry the release.
+After an unclean exit or release failure, use the paired Server's administrator
+recovery procedure below instead of looking for a token in Loader logs.
+
+Hubble's existing task lifecycle can finish, stop or reconnect without calling
+`Loader.shutdown()`. This SDK change alone can therefore leave active owners and
+block mode changes, including restore. Hubble integration requires the paired
+[task cleanup fix #814](https://github.com/apache/hugegraph-toolchain/pull/814) and
+[application shutdown fix #818](https://github.com/apache/hugegraph-toolchain/pull/818);
+deploy those fixes together. If owners are already stranded, use the recovery
+steps below.
+
+1. Stop all related imports and ordinary graph writes, and drain their in-flight
+ requests. The Server recovery endpoint cannot perform this external drain.
+2. As an administrator with `ADMIN` or `ROOT` permission, GET
+ `/graphspaces/{graphspace}/graphs/{graph}/mode/owners` on the affected live
+ Server instance. Read its current `instance`, `epoch` and non-secret `revision`
+ immediately before recovery; do not reuse values from another instance or an
+ earlier attempt.
+3. PUT the following body to that same endpoint, replacing the placeholders with
+ the values just read. This recovery action does not require an owner token.
+
+ ```json
+ {
+ "action": "recover",
+ "instance": "<current instance UUID>",
+ "epoch": 0,
+ "revision": 0,
+ "confirm": "I'm sure all imports have stopped"
+ }
+ ```
+
+ Replace both example `0` values with the GET response's current `epoch` and
+ `revision`. Recovery requires both values to match; any owner or mode change
+ after GET makes this request stale.
+ Success returns `state: recovered`, `mode: NONE` and the incremented epoch,
+ clears existing owners, and prevents old-epoch acquire/release requests from
+ taking effect. A stale recovery request returns HTTP 409 so it cannot clear a
+ newer owner, including one acquired within the same epoch. Ordinary graph-status
+ write permission is insufficient (HTTP 403).
+4. If the response is lost or the result is otherwise unknown, inspect the graph
+ mode and capability. Do not blindly obtain a new epoch and repeat recovery,
+ which could clear owners created after the first attempt.
+
+This procedure also clears a manually set `RESTORING` or `MERGING` mode once all
+related work has stopped and drained. It depends on the paired Server change
+linked above; older Servers retain their legacy enum-mode recovery behavior.
+An enum PUT to `NONE` remains rejected while protocol-1 owners are active.
Review Comment:
The Loader README now states that every enum graph-mode PUT is rejected for the graph while protocol-1 owners are active, including NONE, LOADING and RESTORING. It explicitly names the Spark connector and older Loaders and tells users to avoid concurrent enum-path imports, stop and drain the owned imports, release or recover ownership, then retry through the existing client workflow. This clarifies the paired Server's existing contract; it does not add an ownership protocol to Spark or change older Loader implementations.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4262660284
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,27 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
+ } catch (ServerException e) {
+ if (e.status() != 404 || !"HTTP 404 Not Found".equals(e.getMessage()) ||
+ !("jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "javax.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class jakarta.ws.rs.NotFoundException".equals(e.exception()) ||
+ "class javax.ws.rs.NotFoundException".equals(e.exception()))) {
+ throw e;
+ }
+ LOG.warn("Server has no loading-owner endpoint; use legacy mode without concurrent-import protection");
+ }
+ }
+ if (this.loadingOwner != null) {
+ this.client.graphs().acquireLoadingOwner(graph, this.loadingOwner);
Review Comment:
Important: on a paired Server an ordinary import now fails when the graph mode was last set through the enum `PUT .../mode`, and the Loader itself produces that state. A `--restore` or Graph-source import calls `setRestoreMode()` and never resets it: `unsetLoadingMode()` returns early because `loadingOwner` is null and `loadingModeSet` is false, and `LegacyServerCompatibilityTest` asserts the mode is still `RESTORING` after shutdown. The next ordinary import then reaches this `acquireLoadingOwner`, and `LoadingMode.update` at apache/hugegraph#3331 (4a42a77, line 82) rejects it with `Cannot acquire loading ownership of manually set graph mode RESTORING` (an `E.checkState`, so the Loader sees a `ServerException`) because no owner is active and the mode is not `NONE`. At the base commit `setLoadingMode()` wrote `LOADING` unconditionally and cleanup reset it to `NONE`, so restore followed by an ordinary import worked. The same failure hits a graph left in `LOADING` by anything still o
n the enum path, such as a Loader on the previous release or `HGLoadContext` in the Spark connector. Refusing may be the safer choice, but it is a behaviour change that neither the README section nor the PR description mentions. Requested change: state in the README that an ordinary import requires mode `NONE` or owner-held `LOADING` on a protocol-1 Server and how to clear a leftover mode, or reset `RESTORING` when a restore import finishes, and add a `LoadContextTest` case where the stub Server answers the acquire with that rejection.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadContext.java:
##########
@@ -211,6 +215,27 @@ public void updateSchemaCache() {
@SneakyThrows
public synchronized void setLoadingMode() {
String graph = this.client.graph().graph();
+ if (this.loadingOwnerReleased) {
+ throw new LoadException("Loading owner has already been released; create a new context");
+ }
+ if (this.loadingOwner == null) {
+ try {
+ this.loadingOwner = this.client.graphs().newLoadingOwner(graph);
Review Comment:
Important: the handle is created here and acquired below without being logged, so the only place the `instance/epoch/token` tuple ever appears is the `LoadException` thrown when a release fails. In the cases where no release is attempted nothing records it: the Loader is killed (SIGKILL, OOM kill) or `stopThenShutdown()` leaves `tasksStopped` false because `manager.shutdown()` threw, which skips `unsetLoadingMode()`. The README recovery step (line 196) needs the original tuple, and at apache/hugegraph#3331 (4a42a77) `LoadingMode.mode(GraphMode)` rejects the enum `PUT` to `NONE` while an owner is active, so after such an exit the graph stays in `LOADING` until the graph instance is reopened or the Server restarts. Before this PR the same crash left a `LOADING` mode that one enum `PUT` to `NONE` cleared, and Loader users get the new path automatically once the Server supports it. Requested change: log the tuple at INFO right after `newLoadingOwner` returns (release also checks the a
uthenticated principal, so the token alone does not release it), and point the README recovery paragraph at that log line.
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LegacyServerCompatibilityTest.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.loader.test.functional;
+
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+import org.apache.hugegraph.driver.HugeClient;
+import org.apache.hugegraph.exception.ServerException;
+import org.apache.hugegraph.loader.HugeGraphLoader;
+import org.apache.hugegraph.structure.constant.GraphMode;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.util.JsonUtil;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import com.google.common.collect.ImmutableMap;
+
+/** Run explicitly against the official Server 1.7 fixture. */
+public class LegacyServerCompatibilityTest {
Review Comment:
Important: this test is not run by any build or CI job. Surefire in `hugegraph-loader/pom.xml` includes only `${test-classes}`, and the profiles set that to `**/UnitTestSuite.java`, `**/FileLoadTest.java`, `**/HDFSLoadTest.java`, `**/JDBCLoadTest.java` or `**/KafkaLoadTest.java`; `loader-ci.yml` runs exactly those five profiles on both the `candidate` and `release-1.7` legs, and no workflow passes `-Dtest=LegacyServerCompatibilityTest`. The comment on line 38 says it is run explicitly, so it only runs when someone starts it by hand. The ordinary-import fallback is still exercised indirectly, because `FileLoadTest` on the `release-1.7` leg goes through `setLoadingMode()` against the real Server, but the two things this class pins down directly are not: that `newLoadingOwner` gets a 404 from Server 1.7, and that a restore import leaves `RESTORING` with the context closed (`FileLoadTest` has no restore case). Requested change: run it on the `release-1.7` leg of `loader-ci.yml` (a pro
file or an explicit `-Dtest=` step with `hugegraph.loader.test-server-url`), or say in the PR that it is manual-only and why the fixture cannot host it.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4264259611
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LegacyServerCompatibilityTest.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.loader.test.functional;
+
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+import org.apache.hugegraph.driver.HugeClient;
+import org.apache.hugegraph.exception.ServerException;
+import org.apache.hugegraph.loader.HugeGraphLoader;
+import org.apache.hugegraph.structure.constant.GraphMode;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.util.JsonUtil;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import com.google.common.collect.ImmutableMap;
+
+/** Run explicitly against the official Server 1.7 fixture. */
+public class LegacyServerCompatibilityTest {
Review Comment:
The release-1.7 leg now explicitly runs LegacyServerCompatibilityTest with the configured test-server URL before the existing profiles. The test was also run against the official ASF 1.7 distribution after signature and SHA512 verification: the owner endpoint returns 404, ordinary import falls back correctly, and restore leaves RESTORING with its context closed. The isolated Server was stopped afterward; the new-head CI leg still needs its own terminal result.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(loader): guard concurrent graph loading [hugegraph-toolchain]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #815:
URL: https://github.com/apache/hugegraph-toolchain/pull/815#discussion_r4269195426
##########
hugegraph-loader/README.md:
##########
@@ -171,3 +171,67 @@ Server 1.7 compatibility CI uses the pinned official ASF 1.7.0 binary archive. T
hugegraph-loader is licensed under Apache 2.0 License.
See the [Flink CDC Java 17 validation](docs/flink-cdc-java17.md) for its current limits and launcher syntax.
+
+### Concurrent imports and graph mode
Review Comment:
Minor: this section lands under `## 5. License`, after the license sentence and the Flink CDC link, so the README now shows the ownership protocol and the administrator recovery steps as a subsection of License. Someone reading `#### Graph mode during startup and cleanup` in 3.1 (line 136), which now opens with "On Servers without the loading-owner endpoint", has no pointer to the protocol-1 behaviour, and the release-failure message in `LoadContext.unsetLoadingMode()` sends users to a recovery procedure they will not find under 3.1 or 4. Requested change: move this block into 3.1 as a `####` subsection right after "Graph mode during startup and cleanup" (or link to it from line 138), and leave `## 5. License` with only the license text.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org