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: [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]
