You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "Tsukilc (via GitHub)" <gi...@apache.org> on 2026/01/11 11:40:01 UTC
[PR] fix(server): disable server-role in StandardTaskScheduler [incubator-hugegraph]
Tsukilc opened a new pull request, #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937
Removed the scheduling logic of StandardScheduler. For compatibility reasons, serverInfo is temporarily retained.
Removed master-role related configuration items; server-id now uses the default value.
Removed the task.scheduler_type parameter. The scheduler type now follows the storage type by default: distributed for hstore, otherwise local.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc closed pull request #2937: fix(server): fix the scheduler and the scheduler selection logic
URL: https://github.com/apache/hugegraph/pull/2937
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3345702283
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
- } else {
- return this.deleteFromDB(id);
+ // Already in DELETING status, delete directly from DB
+ // Completed tasks can also be deleted directly
Review Comment:
The two comments after `return null;` in `delete()` are unreachable, which is misleading and makes the control flow harder to follow.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -19,7 +19,9 @@
import java.util.Iterator;
import java.util.concurrent.Callable;
+import java.util.concurrent.CancellationException;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
Review Comment:
`CancellationException` is imported but never used. This can break compilation in builds that enforce unused-import checks (and adds noise even when not enforced).
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1937,26 +1938,29 @@ public Set<String> getServiceUrls(String graphSpace, String service,
public HugeGraph graph(String graphSpace, String name) {
String key = String.join(DELIMITER, graphSpace, name);
Graph graph = this.graphs.get(key);
- if (graph == null && isPDEnabled()) {
- Map<String, Map<String, Object>> configs =
- this.metaManager.graphConfigs(graphSpace);
- // If current server registered graph space is not DEFAULT, only load graph creation
- // under registered graph space
- if (!configs.containsKey(key) ||
- (!"DEFAULT".equals(this.serviceGraphSpace) &&
- !graphSpace.equals(this.serviceGraphSpace))) {
- return null;
+ if (graph == null) {
+ if (isPDEnabled()) {
+ Map<String, Map<String, Object>> configs =
+ this.metaManager.graphConfigs(graphSpace);
+ // If current server registered graph space is not DEFAULT, only load graph creation
+ // under registered graph space
+ if (!configs.containsKey(key) ||
+ (!"DEFAULT".equals(this.serviceGraphSpace) &&
+ !graphSpace.equals(this.serviceGraphSpace))) {
+ return null;
+ }
+ Map<String, Object> config = configs.get(key);
+ String creator = String.valueOf(config.get("creator"));
+ Date createTime = parseDate(config.get("create_time"));
+ Date updateTime = parseDate(config.get("update_time"));
+ HugeGraph graph1 = this.createGraph(graphSpace, name,
+ creator, config, false);
+ graph1.createTime(createTime);
+ graph1.updateTime(updateTime);
+ this.graphs.put(key, graph1);
+ return graph1;
}
- Map<String, Object> config = configs.get(key);
- String creator = String.valueOf(config.get("creator"));
- Date createTime = parseDate(config.get("create_time"));
- Date updateTime = parseDate(config.get("update_time"));
- HugeGraph graph1 = this.createGraph(graphSpace, name,
- creator, config, false);
- graph1.createTime(createTime);
- graph1.updateTime(updateTime);
- this.graphs.put(key, graph1);
- return graph1;
+ throw new NotFoundException(String.format("Graph '%s' does not exist", name));
Review Comment:
`graph(graphSpace, name)` now throws `NotFoundException` only when PD is disabled, but returns `null` when PD is enabled and the graph is missing. This mixed contract makes callers harder to reason about and defeats existing `null` checks (e.g. `API.graph()` already handles the not-found case). Consider keeping this method consistently nullable and letting API layers translate `null` to 404.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/masterelection/StandardRoleListener.java:
##########
@@ -17,12 +17,12 @@
package org.apache.hugegraph.masterelection;
-import java.util.Objects;
-
import org.apache.hugegraph.task.TaskManager;
import org.apache.hugegraph.util.Log;
import org.slf4j.Logger;
+import java.util.Objects;
+
Review Comment:
Import grouping/order is inconsistent with the surrounding codebase (java.* imports typically come before org.* imports, with a blank line separating groups). This can cause checkstyle/formatting failures if enforced.
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2703105728
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
Review Comment:
The delete method calls taskWithoutResult which will throw NotFoundException if the task doesn't exist (as per the change in TaskAndResultScheduler.java lines 223-225). This could cause unexpected exceptions when trying to delete a non-existent task. Consider adding error handling or checking for task existence before calling taskWithoutResult.
```suggestion
HugeTask<?> task;
try {
task = this.taskWithoutResult(id);
} catch (NotFoundException e) {
// Task does not exist, treat delete as a no-op
return null;
}
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -629,46 +642,51 @@ public void testGremlinJobAndRestore() throws Exception {
scheduler.cancel(task);
task = scheduler.task(task.id());
- Assert.assertEquals(TaskStatus.CANCELLING, task.status());
+ Assert.assertTrue("Task status should be CANCELLING or CANCELLED, but was " + task.status(),
+ task.status() == TaskStatus.CANCELLING ||
+ task.status() == TaskStatus.CANCELLED);
task = scheduler.waitUntilTaskCompleted(task.id(), 10);
Assert.assertEquals(TaskStatus.CANCELLED, task.status());
Assert.assertTrue("progress=" + task.progress(),
0 < task.progress() && task.progress() < 10);
Assert.assertEquals(0, task.retries());
- Assert.assertEquals(null, task.result());
+ Assert.assertNull(task.result());
HugeTask<Object> finalTask = task;
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- Whitebox.invoke(scheduler.getClass(), "restore", scheduler,
- finalTask);
- }, e -> {
- Assert.assertContains("No need to restore completed task",
- e.getMessage());
- });
- HugeTask<Object> task2 = scheduler.task(task.id());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
+ // because Distributed do nothing in restore, so only test StandardTaskScheduler here
+ if (scheduler.getClass().equals(StandardTaskScheduler.class)) {
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ Whitebox.invoke(scheduler.getClass(), "restore", scheduler,
+ finalTask);
+ }, e -> {
+ Assert.assertContains("No need to restore completed task",
+ e.getMessage());
+ });
+
+ HugeTask<Object> task2 = scheduler.task(task.id());
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ Whitebox.invoke(scheduler.getClass(), "restore", scheduler, task2);
+ }, e -> {
+ Assert.assertContains("No need to restore completed task",
+ e.getMessage());
+ });
+
+ Whitebox.setInternalState(task2, "status", TaskStatus.RUNNING);
Whitebox.invoke(scheduler.getClass(), "restore", scheduler, task2);
- }, e -> {
- Assert.assertContains("No need to restore completed task",
- e.getMessage());
- });
-
- Whitebox.setInternalState(task2, "status", TaskStatus.RUNNING);
- Whitebox.invoke(scheduler.getClass(), "restore", scheduler, task2);
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- Whitebox.invoke(scheduler.getClass(), "restore", scheduler, task2);
- }, e -> {
- Assert.assertContains("is already in the queue", e.getMessage());
- });
-
- scheduler.waitUntilTaskCompleted(task2.id(), 10);
- sleepAWhile(500);
- Assert.assertEquals(10, task2.progress());
- Assert.assertEquals(1, task2.retries());
- Assert.assertEquals("100", task2.result());
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ Whitebox.invoke(scheduler.getClass(), "restore", scheduler, task2);
+ }, e -> {
+ Assert.assertContains("is already in the queue", e.getMessage());
+ });
+ scheduler.waitUntilTaskCompleted(task2.id(), 10);
+ sleepAWhile(500);
+ Assert.assertEquals(10, task2.progress());
+ Assert.assertEquals(1, task2.retries());
+ Assert.assertEquals("100", task2.result());
+ }
Review Comment:
Using class equality check with .equals(StandardTaskScheduler.class) is fragile and could break if subclasses of StandardTaskScheduler are introduced. Consider using instanceof operator instead for better maintainability and to properly handle inheritance hierarchies.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed() && task.status() != TaskStatus.DELETING) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
- } else {
- return this.deleteFromDB(id);
+ // Already in DELETING status, delete directly from DB
+ // Completed tasks can also be deleted directly
}
+
Review Comment:
The comment on lines 364-365 is misplaced. These lines are unreachable code as they appear after a return statement on line 363. The comment should either be moved before the if statement or removed if it's no longer accurate.
```suggestion
}
// Already in DELETING status, delete directly from DB
// Completed tasks can also be deleted directly
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -629,46 +642,51 @@ public void testGremlinJobAndRestore() throws Exception {
scheduler.cancel(task);
task = scheduler.task(task.id());
- Assert.assertEquals(TaskStatus.CANCELLING, task.status());
+ Assert.assertTrue("Task status should be CANCELLING or CANCELLED, but was " + task.status(),
+ task.status() == TaskStatus.CANCELLING ||
+ task.status() == TaskStatus.CANCELLED);
task = scheduler.waitUntilTaskCompleted(task.id(), 10);
Assert.assertEquals(TaskStatus.CANCELLED, task.status());
Assert.assertTrue("progress=" + task.progress(),
0 < task.progress() && task.progress() < 10);
Assert.assertEquals(0, task.retries());
- Assert.assertEquals(null, task.result());
+ Assert.assertNull(task.result());
HugeTask<Object> finalTask = task;
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- Whitebox.invoke(scheduler.getClass(), "restore", scheduler,
- finalTask);
- }, e -> {
- Assert.assertContains("No need to restore completed task",
- e.getMessage());
- });
- HugeTask<Object> task2 = scheduler.task(task.id());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
+ // because Distributed do nothing in restore, so only test StandardTaskScheduler here
Review Comment:
The comment on line 658 says "because Distributed do nothing in restore" but this appears to be documenting implementation details that may change. Consider making this comment more generic or explaining why the different schedulers have different restore behavior from a design perspective rather than implementation perspective.
```suggestion
// Only verify restore behavior for StandardTaskScheduler; other scheduler
// implementations may handle restore differently or not expose it in the
// same way, so they are not covered by this test.
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +296,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
Review Comment:
The cancel method now updates the task status directly by calling task.overwriteStatus(TaskStatus.CANCELLED) on line 320 when the task is running locally. However, this doesn't persist the status change to the database. The task status should be saved to ensure consistency between in-memory state and persisted state.
```suggestion
TaskStatus previousStatus = task.status();
boolean cancelled = runningTask.cancel(true);
if (cancelled) {
if (this.updateStatus(task.id(), previousStatus,
TaskStatus.CANCELLED)) {
task.overwriteStatus(TaskStatus.CANCELLED);
} else {
LOG.info("Failed to persist cancelled status for task '{}', " +
"status may have changed from {}",
task.id(), previousStatus);
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -273,39 +247,22 @@ public <V> void initTaskCallable(HugeTask<V> task) {
@Override
public synchronized <V> void cancel(HugeTask<V> task) {
E.checkArgumentNotNull(task, "Task can't be null");
- this.checkOnMasterNode("cancel");
if (task.completed() || task.cancelling()) {
return;
}
LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
- if (task.server() == null) {
- // The task not scheduled to workers, set canceled immediately
- assert task.status().code() < TaskStatus.QUEUED.code();
- if (task.status(TaskStatus.CANCELLED)) {
- this.save(task);
- return;
- }
- } else if (task.status(TaskStatus.CANCELLING)) {
- // The task scheduled to workers, let the worker node to cancel
+ HugeTask<?> memTask = this.tasks.get(task.id());
+ if (memTask != null) {
+ boolean cancelled = memTask.cancel(true);
+ LOG.info("Task '{}' cancel result: {}", task.id(), cancelled);
+ return;
+ }
+
+ if (task.status(TaskStatus.CANCELLED)) {
this.save(task);
- assert task.server() != null : task;
- assert this.serverManager().selfIsMaster();
- if (!task.server().equals(this.serverManager().selfNodeId())) {
- /*
- * Remove the task from memory if it's running on worker node,
- * but keep the task in memory if it's running on master node.
- * Cancel-scheduling will read the task from backend store, if
- * removed this instance from memory, there will be two task
- * instances with the same id, and can't cancel the real task that
- * is running but removed from memory.
- */
- this.remove(task);
- }
- // Notify master server to schedule and execute immediately
- TaskManager.instance().notifyNewTask(task);
return;
}
Review Comment:
The cancel method in StandardTaskScheduler now tries to cancel the task from memory first and falls back to marking it as CANCELLED. However, if the task is not in memory and status() returns false (meaning the status couldn't be updated), the method returns without any indication of failure or retry logic. This could lead to silent failures where tasks are not properly cancelled.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -253,6 +261,10 @@ public <V> Future<?> schedule(HugeTask<V> task) {
return this.ephemeralTaskExecutor.submit(task);
}
+ // Validate task state before saving to ensure correct exception type
+ E.checkState(task.type() != null, "Task type can't be null");
+ E.checkState(task.name() != null, "Task name can't be null");
Review Comment:
The validation checks on lines 265-266 are performed after saving ephemeral tasks but before saving persistent tasks. This inconsistency could lead to ephemeral tasks being created with null type or name. Consider moving these validation checks before the ephemeral task check on line 260 to ensure all tasks are validated consistently.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -76,20 +77,22 @@ public void testTask() throws TimeoutException {
Assert.assertEquals(id, task.id());
Assert.assertFalse(task.completed());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- scheduler.delete(id, false);
- }, e -> {
- Assert.assertContains("Can't delete incomplete task '88888'",
- e.getMessage());
- });
+ if (scheduler.getClass().equals(StandardTaskScheduler.class)) {
+ Assert.assertThrows(IllegalArgumentException.class, () -> {
+ scheduler.delete(id, false);
+ }, e -> {
+ Assert.assertContains("Can't delete incomplete task '88888'",
+ e.getMessage());
+ });
+ }
Review Comment:
Using class equality check with .equals(StandardTaskScheduler.class) is fragile and could break if subclasses of StandardTaskScheduler are introduced. Consider using instanceof operator instead for better maintainability and to properly handle inheritance hierarchies.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -738,10 +578,9 @@ public <V> V call(Callable<V> callable) {
}
}
+ @Deprecated
private void checkOnMasterNode(String op) {
- if (!this.serverManager().selfIsMaster()) {
- throw new HugeException("Can't %s task on non-master server", op);
- }
+ // Single-node mode: all operations are allowed, no role check needed
}
Review Comment:
The checkOnMasterNode method is marked as @Deprecated but is still being called in the codebase. While it now has no-op implementation, deprecated methods that are no longer needed should be removed entirely, or the deprecation should include guidance on what to use instead with @deprecated JavaDoc tag.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -363,7 +418,10 @@ public boolean close() {
this.graph.closeTx();
});
}
- return true;
+
+ //todo: serverInfoManager section should be removed in the future.
Review Comment:
The comment on line 422 has inconsistent formatting. Comments should follow a consistent style. Either use "TODO" (uppercase) or "todo" (lowercase) consistently throughout the codebase. The standard convention is "TODO" with a colon.
```suggestion
// TODO: serverInfoManager section should be removed in the future.
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -1629,7 +1629,8 @@ public <T> void submitEphemeralJob(EphemeralJob<T> job) {
@Override
public String schedulerType() {
- return StandardHugeGraph.this.schedulerType;
+ // Use distributed scheduler for hstore backend, otherwise use local
Review Comment:
The schedulerType() method now determines the scheduler type based on whether the backend is hstore. However, this logic change is undocumented. Consider adding a comment explaining why hstore backends require distributed scheduling while other backends use local scheduling, as this is an important architectural decision.
```suggestion
/*
* HStore is a distributed backend: data and tasks may be handled by
* multiple graph servers that must coordinate scheduling and state.
* For this reason we require a distributed task scheduler when the
* backend is hstore so that jobs can be balanced and recovered
* across nodes. For other backends, the graph is served by a single
* server instance and tasks are executed locally, so a local
* in-process scheduler is sufficient and avoids the overhead of
* distributed coordination.
*/
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -363,7 +418,10 @@ public boolean close() {
this.graph.closeTx();
});
}
- return true;
+
+ //todo: serverInfoManager section should be removed in the future.
+ return this.serverManager().close();
+ //return true;
Review Comment:
The commented-out return statement on line 424 should be removed. Keeping alternative code as comments can be confusing and clutters the codebase. If this is temporary code during a transition period, consider adding a more descriptive TODO comment explaining the transition plan.
```suggestion
```
--
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(server): disable server-role in StandardTaskScheduler [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2681005122
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -273,173 +241,39 @@ public <V> void initTaskCallable(HugeTask<V> task) {
@Override
public synchronized <V> void cancel(HugeTask<V> task) {
E.checkArgumentNotNull(task, "Task can't be null");
- this.checkOnMasterNode("cancel");
if (task.completed() || task.cancelling()) {
return;
}
LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
- if (task.server() == null) {
- // The task not scheduled to workers, set canceled immediately
- assert task.status().code() < TaskStatus.QUEUED.code();
- if (task.status(TaskStatus.CANCELLED)) {
- this.save(task);
- return;
- }
- } else if (task.status(TaskStatus.CANCELLING)) {
- // The task scheduled to workers, let the worker node to cancel
+ HugeTask<?> memTask = this.tasks.get(task.id());
+ if (memTask != null) {
+ boolean cancelled = memTask.cancel(true);
+ LOG.info("Task '{}' cancel result: {}", task.id(), cancelled);
Review Comment:
The cancel method no longer saves the task to storage when cancelling a task that's in memory. If the task is running in memory and gets cancelled via memTask.cancel(true), the cancelled status won't be persisted to storage until the task naturally completes. This could cause issues if the server restarts before the task finishes - the task would be restored and re-executed. Consider saving the task status to storage after successfully calling memTask.cancel(true).
```suggestion
LOG.info("Task '{}' cancel result: {}", task.id(), cancelled);
if (cancelled) {
this.save(memTask);
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -146,19 +137,17 @@ private TaskTransaction tx() {
@Override
public <V> void restoreTasks() {
- Id selfServer = this.serverManager().selfNodeId();
List<HugeTask<V>> taskList = new ArrayList<>();
// Restore 'RESTORING', 'RUNNING' and 'QUEUED' tasks in order.
+ // Single-node mode: restore all pending tasks without server filtering
Review Comment:
The comment mentions "Single-node mode" but this change applies regardless of deployment mode. Since the PR description states that the scheduler type now follows storage type (distributed for hstore, otherwise local), this comment should be updated to accurately reflect that server filtering has been removed, not just for single-node deployments.
```suggestion
// Restore all pending tasks without server filtering (applies to all deployment modes)
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -739,9 +573,7 @@ public <V> V call(Callable<V> callable) {
}
Review Comment:
The checkOnMasterNode method now only contains a comment and no longer performs any validation. While this change aligns with the PR's goal of removing master-role logic, the method is still being called in the codebase. Consider either removing this method entirely or at least adding a deprecation notice if it's being kept for API compatibility.
```suggestion
/**
* Check whether the current node is allowed to perform the specified
* operation. This method previously enforced master-role validation but
* now performs no checks in single-node mode.
*
* @deprecated This method no longer performs any validation and is kept
* only for API compatibility. New code should not call it.
*/
@Deprecated
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -273,173 +241,39 @@ public <V> void initTaskCallable(HugeTask<V> task) {
@Override
public synchronized <V> void cancel(HugeTask<V> task) {
E.checkArgumentNotNull(task, "Task can't be null");
- this.checkOnMasterNode("cancel");
if (task.completed() || task.cancelling()) {
return;
}
LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
- if (task.server() == null) {
- // The task not scheduled to workers, set canceled immediately
- assert task.status().code() < TaskStatus.QUEUED.code();
- if (task.status(TaskStatus.CANCELLED)) {
- this.save(task);
- return;
- }
- } else if (task.status(TaskStatus.CANCELLING)) {
- // The task scheduled to workers, let the worker node to cancel
+ HugeTask<?> memTask = this.tasks.get(task.id());
+ if (memTask != null) {
+ boolean cancelled = memTask.cancel(true);
+ LOG.info("Task '{}' cancel result: {}", task.id(), cancelled);
+ return;
+ }
+
+ if (task.status(TaskStatus.CANCELLED)) {
this.save(task);
- assert task.server() != null : task;
- assert this.serverManager().selfIsMaster();
- if (!task.server().equals(this.serverManager().selfNodeId())) {
- /*
- * Remove the task from memory if it's running on worker node,
- * but keep the task in memory if it's running on master node.
- * Cancel-scheduling will read the task from backend store, if
- * removed this instance from memory, there will be two task
- * instances with the same id, and can't cancel the real task that
- * is running but removed from memory.
- */
- this.remove(task);
- }
- // Notify master server to schedule and execute immediately
- TaskManager.instance().notifyNewTask(task);
return;
}
throw new HugeException("Can't cancel task '%s' in status %s",
- task.id(), task.status());
+ task.id(), task.status());
}
@Override
public ServerInfoManager serverManager() {
return this.serverManager;
}
- protected synchronized void scheduleTasksOnMaster() {
- // Master server schedule all scheduling tasks to suitable worker nodes
- Collection<HugeServerInfo> serverInfos = this.serverManager().allServerInfos();
- String page = this.supportsPaging() ? PageInfo.PAGE_NONE : null;
- do {
- Iterator<HugeTask<Object>> tasks = this.tasks(TaskStatus.SCHEDULING, PAGE_SIZE, page);
- while (tasks.hasNext()) {
- HugeTask<?> task = tasks.next();
- if (task.server() != null) {
- // Skip if already scheduled
- continue;
- }
-
- if (!this.serverManager.selfIsMaster()) {
- return;
- }
-
- HugeServerInfo server = this.serverManager().pickWorkerNode(serverInfos, task);
- if (server == null) {
- LOG.info("The master can't find suitable servers to " +
- "execute task '{}', wait for next schedule", task.id());
- continue;
- }
-
- // Found suitable server, update task status
- assert server.id() != null;
- task.server(server.id());
- task.status(TaskStatus.SCHEDULED);
- this.save(task);
-
- // Update server load in memory, it will be saved at the ending
- server.increaseLoad(task.load());
-
- LOG.info("Scheduled task '{}' to server '{}'", task.id(), server.id());
- }
- if (page != null) {
- page = PageInfo.pageInfo(tasks);
- }
- } while (page != null);
-
- // Save to store
- this.serverManager().updateServerInfos(serverInfos);
- }
-
- protected void executeTasksOnWorker(Id server) {
- String page = this.supportsPaging() ? PageInfo.PAGE_NONE : null;
- do {
- Iterator<HugeTask<Object>> tasks = this.tasks(TaskStatus.SCHEDULED, PAGE_SIZE, page);
- while (tasks.hasNext()) {
- HugeTask<?> task = tasks.next();
- this.initTaskCallable(task);
- Id taskServer = task.server();
- if (taskServer == null) {
- LOG.warn("Task '{}' may not be scheduled", task.id());
- continue;
- }
- HugeTask<?> memTask = this.tasks.get(task.id());
- if (memTask != null) {
- assert memTask.status().code() > task.status().code();
- continue;
- }
- if (taskServer.equals(server)) {
- task.status(TaskStatus.QUEUED);
- this.save(task);
- this.submitTask(task);
- }
- }
- if (page != null) {
- page = PageInfo.pageInfo(tasks);
- }
- } while (page != null);
- }
-
- protected void cancelTasksOnWorker(Id server) {
- String page = this.supportsPaging() ? PageInfo.PAGE_NONE : null;
- do {
- Iterator<HugeTask<Object>> tasks = this.tasks(TaskStatus.CANCELLING, PAGE_SIZE, page);
- while (tasks.hasNext()) {
- HugeTask<?> task = tasks.next();
- Id taskServer = task.server();
- if (taskServer == null) {
- LOG.warn("Task '{}' may not be scheduled", task.id());
- continue;
- }
- if (!taskServer.equals(server)) {
- continue;
- }
- /*
- * Task may be loaded from backend store and not initialized.
- * like: A task is completed but failed to save in the last
- * step, resulting in the status of the task not being
- * updated to storage, the task is not in memory, so it's not
- * initialized when canceled.
- */
- HugeTask<?> memTask = this.tasks.get(task.id());
- if (memTask != null) {
- task = memTask;
- } else {
- this.initTaskCallable(task);
- }
- boolean cancelled = task.cancel(true);
- LOG.info("Server '{}' cancel task '{}' with cancelled={}",
- server, task.id(), cancelled);
- }
- if (page != null) {
- page = PageInfo.pageInfo(tasks);
- }
- } while (page != null);
- }
-
@Override
public void taskDone(HugeTask<?> task) {
this.remove(task);
-
- Id selfServerId = this.serverManager().selfNodeId();
- try {
- this.serverManager().decreaseLoad(task.load());
- } catch (Throwable e) {
- LOG.error("Failed to decrease load for task '{}' on server '{}'",
- task.id(), selfServerId, e);
- }
- LOG.debug("Task '{}' done on server '{}'", task.id(), selfServerId);
+ // Single-node mode: no need to manage load
Review Comment:
The comment mentions "Single-node mode" but this scheduler can now also be used in distributed mode (when storage type is hstore). The comment should be updated to reflect that this is a simplified scheduler that doesn't manage server load, regardless of deployment mode.
```suggestion
// Simplified scheduler: does not manage server load in any mode
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -557,13 +553,16 @@ public void testGremlinJobAndCancel() throws TimeoutException {
scheduler.cancel(task);
task = scheduler.task(task.id());
- Assert.assertEquals(TaskStatus.CANCELLING, task.status());
+ System.out.println(scheduler.getClass());
Review Comment:
Debug output should be removed. This System.out.println statement was likely added during debugging and should not be committed to production code.
```suggestion
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -32,17 +29,16 @@
import org.apache.hugegraph.job.EphemeralJobBuilder;
import org.apache.hugegraph.job.GremlinJob;
import org.apache.hugegraph.job.JobBuilder;
-import org.apache.hugegraph.task.HugeTask;
-import org.apache.hugegraph.task.TaskCallable;
-import org.apache.hugegraph.task.TaskScheduler;
-import org.apache.hugegraph.task.TaskStatus;
+import org.apache.hugegraph.task.*;
Review Comment:
Wildcard import should be avoided. Replace "import org.apache.hugegraph.task.*;" with explicit imports for the specific classes being used (HugeTask, TaskCallable, TaskScheduler, TaskStatus, DistributedTaskScheduler).
```suggestion
import org.apache.hugegraph.task.DistributedTaskScheduler;
import org.apache.hugegraph.task.HugeTask;
import org.apache.hugegraph.task.TaskCallable;
import org.apache.hugegraph.task.TaskScheduler;
import org.apache.hugegraph.task.TaskStatus;
```
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2804208363
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +296,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
+ if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
Review Comment:
‼️ 这里使用 `currentStatus` 做 CAS 更新有明显竞态：调用方读取任务后到执行 `cancel()` 之间，任务可能从 `QUEUED` 变成 `RUNNING`，导致 `updateStatus(..., currentStatus, CANCELLING)` 失败并直接返回，取消请求被静默丢弃。建议 CAS 失败后读取最新状态并做一次兜底更新（只要未完成就转 `CANCELLING`）。
```suggestion
if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
HugeTask<?> latest = this.taskWithoutResult(task.id());
if (!latest.completed() && !latest.cancelling()) {
this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
task.overwriteStatus(TaskStatus.CANCELLING);
}
} else {
task.overwriteStatus(TaskStatus.CANCELLING);
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -211,30 +206,9 @@ public <V> Future<?> schedule(HugeTask<V> task) {
return this.submitTask(task);
}
- // Check this is on master for normal task schedule
- this.checkOnMasterNode("schedule");
- if (this.serverManager().onlySingleNode() && !task.computer()) {
- /*
- * Speed up for single node, submit the task immediately,
- * this code can be removed without affecting code logic
- */
- task.status(TaskStatus.QUEUED);
- task.server(this.serverManager().selfNodeId());
- this.save(task);
- return this.submitTask(task);
- } else {
- /*
- * Just set the SCHEDULING status and save the task,
- * it will be scheduled by periodic scheduler worker
- */
- task.status(TaskStatus.SCHEDULING);
- this.save(task);
-
- // Notify master server to schedule and execute immediately
- TaskManager.instance().notifyNewTask(task);
-
- return task;
- }
+ task.status(TaskStatus.QUEUED);
Review Comment:
‼️ 这里将普通任务直接置为 `QUEUED` 并提交，但不再写入 `task.server`。结合 `restoreTasks()` 取消按 server 过滤后，在共享后端的多实例场景（如 MySQL/PostgreSQL/Cassandra）重启时，每个节点都可能恢复并执行同一批 pending 任务，产生重复执行。建议保留 owner 绑定，至少让恢复阶段可按 owner 做隔离。
```suggestion
task.status(TaskStatus.QUEUED);
task.server(this.serverManager().selfNodeId());
this.save(task);
return this.submitTask(task);
```
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3367177220
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +296,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
Review Comment:
When canceling a locally running task, the method updates only the in-memory `task` status and returns without persisting the cancellation to the backend store. This can leave the DB task status stale (e.g., still RUNNING/QUEUED), causing incorrect restore/query behavior after restart. Consider persisting the status transition (e.g., via `updateStatus(...)` and/or `save(task)` as appropriate) before returning.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -146,19 +143,17 @@ private TaskTransaction tx() {
@Override
public <V> void restoreTasks() {
- Id selfServer = this.serverManager().selfNodeId();
List<HugeTask<V>> taskList = new ArrayList<>();
// Restore 'RESTORING', 'RUNNING' and 'QUEUED' tasks in order.
+ // Single-node mode: restore all pending tasks without server filtering
for (TaskStatus status : TaskStatus.PENDING_STATUSES) {
String page = this.supportsPaging() ? PageInfo.PAGE_NONE : null;
do {
Iterator<HugeTask<V>> iter;
for (iter = this.findTask(status, PAGE_SIZE, page);
iter.hasNext(); ) {
HugeTask<V> task = iter.next();
- if (selfServer.equals(task.server())) {
- taskList.add(task);
- }
+ taskList.add(task);
}
Review Comment:
`restoreTasks()` now restores *all* pending tasks without filtering by `task.server()`. If `StandardTaskScheduler` can ever run against a shared/persistent backend used by multiple servers (or data migrated from a distributed setup), this risks executing tasks that were assigned to a different server. If the intent is strictly single-node/local mode, consider enforcing that invariant (e.g., assert/config guard) or restore only tasks that are unassigned / match the current node when node identity is available.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1937,26 +1938,29 @@ public Set<String> getServiceUrls(String graphSpace, String service,
public HugeGraph graph(String graphSpace, String name) {
String key = String.join(DELIMITER, graphSpace, name);
Graph graph = this.graphs.get(key);
- if (graph == null && isPDEnabled()) {
- Map<String, Map<String, Object>> configs =
- this.metaManager.graphConfigs(graphSpace);
- // If current server registered graph space is not DEFAULT, only load graph creation
- // under registered graph space
- if (!configs.containsKey(key) ||
- (!"DEFAULT".equals(this.serviceGraphSpace) &&
- !graphSpace.equals(this.serviceGraphSpace))) {
- return null;
+ if (graph == null) {
+ if (isPDEnabled()) {
+ Map<String, Map<String, Object>> configs =
+ this.metaManager.graphConfigs(graphSpace);
+ // If current server registered graph space is not DEFAULT, only load graph creation
+ // under registered graph space
+ if (!configs.containsKey(key) ||
+ (!"DEFAULT".equals(this.serviceGraphSpace) &&
+ !graphSpace.equals(this.serviceGraphSpace))) {
+ return null;
+ }
+ Map<String, Object> config = configs.get(key);
+ String creator = String.valueOf(config.get("creator"));
+ Date createTime = parseDate(config.get("create_time"));
+ Date updateTime = parseDate(config.get("update_time"));
+ HugeGraph graph1 = this.createGraph(graphSpace, name,
+ creator, config, false);
+ graph1.createTime(createTime);
+ graph1.updateTime(updateTime);
+ this.graphs.put(key, graph1);
+ return graph1;
}
- Map<String, Object> config = configs.get(key);
- String creator = String.valueOf(config.get("creator"));
- Date createTime = parseDate(config.get("create_time"));
- Date updateTime = parseDate(config.get("update_time"));
- HugeGraph graph1 = this.createGraph(graphSpace, name,
- creator, config, false);
- graph1.createTime(createTime);
- graph1.updateTime(updateTime);
- this.graphs.put(key, graph1);
- return graph1;
+ throw new NotFoundException(String.format("Graph '%s' does not exist", name));
Review Comment:
This changes `GraphManager.graph()` behavior for missing graphs when PD is disabled: it now throws `NotFoundException` instead of returning `null`, while the PD-enabled path can still return `null` for 'not visible in this service graph space' cases. This inconsistency can break existing callers that treat a missing graph as `null`. Consider standardizing the contract (always return `null`, or always throw a consistent exception), and apply the same behavior across PD-enabled/disabled modes.
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2697764581
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -273,173 +241,39 @@ public <V> void initTaskCallable(HugeTask<V> task) {
@Override
public synchronized <V> void cancel(HugeTask<V> task) {
E.checkArgumentNotNull(task, "Task can't be null");
- this.checkOnMasterNode("cancel");
if (task.completed() || task.cancelling()) {
return;
}
LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
- if (task.server() == null) {
- // The task not scheduled to workers, set canceled immediately
- assert task.status().code() < TaskStatus.QUEUED.code();
- if (task.status(TaskStatus.CANCELLED)) {
- this.save(task);
- return;
- }
- } else if (task.status(TaskStatus.CANCELLING)) {
- // The task scheduled to workers, let the worker node to cancel
+ HugeTask<?> memTask = this.tasks.get(task.id());
+ if (memTask != null) {
+ boolean cancelled = memTask.cancel(true);
+ LOG.info("Task '{}' cancel result: {}", task.id(), cancelled);
Review Comment:
this.callable.cancelled()会负责调用 taskSchduler 持久化
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3331568650
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1557,6 +1551,14 @@ private void loadGraph(String name, String graphConfPath) {
String raftGroupPeers = this.conf.get(ServerOptions.RAFT_GROUP_PEERS);
config.addProperty(ServerOptions.RAFT_GROUP_PEERS.name(),
raftGroupPeers);
+
+ // Transfer `pd.peers` from server config to graph config
+ // Only inject if not already configured in graph config
+ if (!config.containsKey("pd.peers")) {
+ String pdPeers = this.conf.get(ServerOptions.PD_PEERS);
+ config.addProperty("pd.peers", pdPeers);
Review Comment:
Non-blocking: this now injects server-level `pd.peers` into every graph config when the graph config does not contain it. Please guard this with a non-empty/PD-enabled check, or add coverage for standalone RocksDB graphs, so a default or empty server-level PD setting does not silently change graph startup behavior.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3469654413
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +295,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
Review Comment:
**High: Distributed cancellation can be lost on status races**
`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:327`
**Evidence**
- `cancel()` snapshots `task.status()` from the caller object and passes it as `prestatus`; `updateStatus()` then reloads storage and rejects the update if the stored status no longer equals that stale value at lines 523-543.
**Impact**
- If a remote task moves from `NEW` to `RUNNING` between API read and cancellation, the cancel request is dropped and the task keeps running.
**Requested fix**
- Re-read/retry in the cancellation update path and transition any stored non-completed, non-cancelling status to `CANCELLING`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -1632,7 +1627,9 @@ public <T> void submitEphemeralJob(EphemeralJob<T> job) {
@Override
public String schedulerType() {
- return StandardHugeGraph.this.schedulerType;
+ // Use distributed scheduler for hstore backend, otherwise use local
+ // After the merger of rocksdb and hstore, consider whether to change this logic
+ return StandardHugeGraph.this.isHstore() ? "distributed" : "local";
Review Comment:
**High: Non-hstore clustered tasks now use uncoordinated local scheduling**
`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:1632`
**Evidence**
- `schedulerType()` now returns `"local"` for every non-hstore backend, and local scheduling restores every pending task without server filtering at `StandardTaskScheduler.java:149-156` and immediately saves/submits all normal tasks at `StandardTaskScheduler.java:209-211`. The removed code used configured scheduler type, server ownership, master/worker scheduling, and `HugeServerInfo.suitableFor()` role matching.
**Impact**
- Non-hstore deployments sharing a backend can execute the same persisted task on multiple servers, and computer jobs can run on the submitting non-computer server instead of a `server.role=computer` worker.
**Requested fix**
- Keep uncoordinated local scheduling only for truly single-process stores; preserve a configurable/cluster-safe scheduler path with ownership and computer-role routing for shared or multi-server deployments.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -146,19 +143,17 @@ private TaskTransaction tx() {
@Override
public <V> void restoreTasks() {
- Id selfServer = this.serverManager().selfNodeId();
List<HugeTask<V>> taskList = new ArrayList<>();
// Restore 'RESTORING', 'RUNNING' and 'QUEUED' tasks in order.
+ // Single-node mode: restore all pending tasks without server filtering
for (TaskStatus status : TaskStatus.PENDING_STATUSES) {
Review Comment:
**Medium: Legacy `SCHEDULING` and `SCHEDULED` tasks are orphaned after upgrade**
`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:149`
**Evidence**
- Restore only scans `TaskStatus.PENDING_STATUSES`, which is `RESTORING`, `RUNNING`, and `QUEUED`; `SCHEDULING`/`SCHEDULED` remain enum values but the PR removed the periodic master/worker loop that consumed them.
**Impact**
- Tasks persisted in those states before upgrade can remain permanently stuck and never execute or finish.
**Requested fix**
- Add startup migration/drain logic for `SCHEDULING` and `SCHEDULED` tasks, with a regression test using persisted legacy task states.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1635,23 +1645,14 @@ private void checkBackendVersionOrExit(HugeConfig config) {
}
private void initNodeRole() {
- String id = config.get(ServerOptions.SERVER_ID);
+ boolean enableRoleElection = config.get(
+ ServerOptions.ENABLE_SERVER_ROLE_ELECTION);
+ E.checkArgument(!enableRoleElection,
Review Comment:
**Medium: Existing role-election configs now fail startup**
`hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:1650`
**Evidence**
- `server.role_election` remains a public documented option in `ServerOptions.java:45-51`, but `initNodeRole()` rejects `true` with `"The server.role_election is no longer supported"`; the later election startup branch still checks the same option at lines 1665-1667.
**Impact**
- Existing deployments with `server.role_election=true` fail during `GraphManager` construction after upgrade.
**Requested fix**
- Preserve backward-compatible startup behavior, or remove/deprecate the option with an explicit migration path and tests instead of leaving supported-looking config that aborts startup.
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2697146633
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +295,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
+ if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
+ LOG.info("Failed to cancel task '{}', status may have changed from {}",
+ task.id(), currentStatus);
Review Comment:
⚠️ **资源关闭顺序问题 - 可能导致事务泄漏**
在 `DistributedTaskScheduler.close()` 方法第 313-336 行,cron task 等待完成的逻辑在关闭 `taskDbExecutor` 之前:
```java
try {
cronFuture.get(schedulePeriod + 5, TimeUnit.SECONDS);
} catch (CancellationException e) {
LOG.debug("Cron task was cancelled");
} catch (TimeoutException e) {
LOG.warn("Cron task did not complete in time when closing scheduler");
} catch (ExecutionException | InterruptedException e) {
LOG.warn("Exception while waiting for cron task to complete", e);
}
```
**问题**:
1. 如果 cron task 超时,后续的数据库操作可能仍在执行,但 `taskDbExecutor` 已经被关闭
2. `InterruptedException` 被捕获但线程中断状态未恢复
**建议**:
```suggestion
} catch (ExecutionException | InterruptedException e) {
LOG.warn("Exception while waiting for cron task to complete", e);
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt(); // 恢复中断状态
}
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +295,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
Review Comment:
‼️ **业务逻辑设计问题 - delete 方法的强制删除逻辑不一致**
在 `DistributedTaskScheduler.delete()` 方法第 286-305 行,删除逻辑与原实现存在重大差异:
**原逻辑**:
- `force=false`: 设置状态为 DELETING,返回 null
- `force=true`: 直接从数据库删除
**新逻辑**:
```java
if (!force) {
if (!task.completed() && task.status() != TaskStatus.DELETING) {
throw new IllegalArgumentException(
String.format("Can't delete incomplete task '%s' in status %s, " +
"Please try to cancel the task first",
id, task.status()));
}
}
return this.deleteFromDB(id);
```
**问题**:
1. 移除了 DELETING 状态的设置逻辑,这可能破坏依赖定时清理的代码
2. 非强制删除现在会直接删除完成的任务,而不是先标记为 DELETING
3. 与 `StandardTaskScheduler` 的实现可能不一致
**建议**:重新考虑删除流程,保持与原有逻辑的兼容性,或在 PR 描述中明确说明此行为变更
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +295,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
+ if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
+ LOG.info("Failed to cancel task '{}', status may have changed from {}",
+ task.id(), currentStatus);
} else {
- LOG.info("cancel task({}) error, task has completed", task.id());
+ task.overwriteStatus(TaskStatus.CANCELLING);
}
}
Review Comment:
⚠️ **代码可维护性 - TODO 注释应该更具体**
第 334 行的 TODO 注释过于模糊:
```java
//todo: serverInfoManager section should be removed in the future.
return this.serverManager().close();
//return true;
```
**问题**:
1. 未说明为什么要移除 serverInfoManager
2. 未说明移除的时间节点或前提条件
3. 注释掉的代码应该删除,而不是保留
**建议**:
```suggestion
// TODO(issue-XXX): Remove serverInfoManager.close() after migrating to
// pure single-node architecture. Currently kept for backward compatibility.
return this.serverManager().close();
```
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3409185455
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1937,26 +1938,29 @@ public Set<String> getServiceUrls(String graphSpace, String service,
public HugeGraph graph(String graphSpace, String name) {
String key = String.join(DELIMITER, graphSpace, name);
Graph graph = this.graphs.get(key);
- if (graph == null && isPDEnabled()) {
- Map<String, Map<String, Object>> configs =
- this.metaManager.graphConfigs(graphSpace);
- // If current server registered graph space is not DEFAULT, only load graph creation
- // under registered graph space
- if (!configs.containsKey(key) ||
- (!"DEFAULT".equals(this.serviceGraphSpace) &&
- !graphSpace.equals(this.serviceGraphSpace))) {
- return null;
+ if (graph == null) {
+ if (isPDEnabled()) {
+ Map<String, Map<String, Object>> configs =
+ this.metaManager.graphConfigs(graphSpace);
+ // If current server registered graph space is not DEFAULT, only load graph creation
+ // under registered graph space
+ if (!configs.containsKey(key) ||
+ (!"DEFAULT".equals(this.serviceGraphSpace) &&
+ !graphSpace.equals(this.serviceGraphSpace))) {
+ return null;
+ }
+ Map<String, Object> config = configs.get(key);
+ String creator = String.valueOf(config.get("creator"));
+ Date createTime = parseDate(config.get("create_time"));
+ Date updateTime = parseDate(config.get("update_time"));
+ HugeGraph graph1 = this.createGraph(graphSpace, name,
+ creator, config, false);
+ graph1.createTime(createTime);
+ graph1.updateTime(updateTime);
+ this.graphs.put(key, graph1);
+ return graph1;
}
- Map<String, Object> config = configs.get(key);
- String creator = String.valueOf(config.get("creator"));
- Date createTime = parseDate(config.get("create_time"));
- Date updateTime = parseDate(config.get("update_time"));
- HugeGraph graph1 = this.createGraph(graphSpace, name,
- creator, config, false);
- graph1.createTime(createTime);
- graph1.updateTime(updateTime);
- this.graphs.put(key, graph1);
- return graph1;
+ throw new NotFoundException(String.format("Graph '%s' does not exist", name));
Review Comment:
The throw was actually introduced to fix a latent NPE: previously, when PD is disabled and the graph is missing, control fell through to throw new NotSupportException("graph instance of %s", graph.getClass()) with graph == null — i.e. an NPE. The new NotFoundException is a strict improvement. I agree the split between null (PD enabled, not visible) and throw (PD disabled, missing) is an inconsistent contract worth unifying, but doing it safely requires auditing every caller of graph(graphSpace, name) for null-handling, which is broader than this PR's scope. Proposing to unify to a single nullable contract in a dedicated follow-up.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#issuecomment-4703964899
@imbajin @VGalaxies PTAL~
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2804216101
##########
docker/configs/server2-conf/graphs/hugegraph.properties:
##########
@@ -13,7 +13,6 @@ serializer=binary
pd.peers=127.0.0.1:8686,127.0.0.1:8687,127.0.0.1:8688
# task config
-task.scheduler_type=local
task.schedule_period=10
Review Comment:
⚠️ 这里已经移除了 `task.scheduler_type`，但同目录的 `server1-conf` / `server3-conf` 仍保留该配置项。由于本 PR 同时删除了 `CoreOptions.SCHEDULER_TYPE`，这会导致示例配置不一致，并在启动时产生冗余配置告警。建议同步清理另外两个 docker 配置文件，避免误导运维配置。
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#issuecomment-4284357822
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2697740066
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +295,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
+ if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
+ LOG.info("Failed to cancel task '{}', status may have changed from {}",
+ task.id(), currentStatus);
Review Comment:
Even if the interruption occurs, the transaction still needs to be closed.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r2969209968
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1960,7 +1972,7 @@ public HugeGraph graph(String graphSpace, String name) {
} else if (graph instanceof HugeGraph) {
return (HugeGraph) graph;
}
- throw new NotSupportException("graph instance of %s", graph.getClass());
+ throw new NotFoundException(String.format("Graph '%s' does not exist", name));
}
Review Comment:
`graph()` now throws NotFoundException even when a graph entry exists but is not an instance of `HugeGraph` (the prior behavior threw NotSupportException). This can mask configuration/initialization problems as a 404. Consider restoring the previous exception type for the non-`HugeGraph` case, and only throwing NotFound when `graph == null`/missing.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -353,6 +396,18 @@ public boolean close() {
cronFuture.cancel(false);
}
+ // Wait for cron task to complete to ensure all transactions are closed
+ try {
+ cronFuture.get(schedulePeriod + 5, TimeUnit.SECONDS);
+ } catch (CancellationException e) {
+ // Task was cancelled, this is expected
+ LOG.debug("Cron task was cancelled");
+ } catch (TimeoutException e) {
+ LOG.warn("Cron task did not complete in time when closing scheduler");
+ } catch (ExecutionException | InterruptedException e) {
+ LOG.warn("Exception while waiting for cron task to complete", e);
+ }
Review Comment:
`cronFuture.get(...)` will not reliably wait for an in-flight `scheduleWithFixedDelay` execution to finish once the future is cancelled (it typically throws `CancellationException` immediately). That undermines the goal of ensuring cron scheduling has stopped before closing transactions. Consider using a barrier on the single-threaded `schedulerExecutor` (e.g., submit a no-op and wait) or shutting down/awaiting the executor, and ensure `InterruptedException` restores the interrupt flag when caught.
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2804140862
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1557,6 +1565,14 @@ private void loadGraph(String name, String graphConfPath) {
String raftGroupPeers = this.conf.get(ServerOptions.RAFT_GROUP_PEERS);
config.addProperty(ServerOptions.RAFT_GROUP_PEERS.name(),
raftGroupPeers);
+
+ // Transfer `pd.peers` from server config to graph config
+ // Only inject if not already configured in graph config
+ if (!config.containsKey("pd.peers")) {
Review Comment:
The pd.peers configuration is being transferred from server config to graph config (lines 1571-1574), but this happens unconditionally for all graphs during loadGraph. This means even graphs that don't need PD will get this configuration injected. Consider checking if the graph actually uses PD (e.g., by checking if it's an hstore backend) before injecting this configuration, or document why all graphs need this configuration.
```suggestion
// Transfer `pd.peers` from server config to graph config when PD is used
// Only inject if not already configured in graph config and backend uses PD
String backend = config.get(CoreOptions.BACKEND);
boolean backendUsesPd = "hstore".equalsIgnoreCase(backend);
if (backendUsesPd && !config.containsKey("pd.peers")) {
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -363,7 +418,10 @@ public boolean close() {
this.graph.closeTx();
});
}
- return true;
+
+ //todo: serverInfoManager section should be removed in the future.
+ return this.serverManager().close();
+ //return true;
Review Comment:
The TODO comment indicates that serverInfoManager should be removed in the future, but the code returns the result of serverManager().close() instead of just returning true. This creates uncertainty about the return value and whether the close operation succeeded. Consider clarifying the intention: if serverInfoManager is truly being deprecated, document why its close() result is being returned here, or if it's critical, remove the TODO and keep the current implementation clear.
```suggestion
/*
* Propagate the result of serverManager().close() so callers can
* detect whether the underlying server manager shutdown succeeded.
*/
return this.serverManager().close();
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -195,7 +197,17 @@ public final class GraphManager {
public GraphManager(HugeConfig conf, EventHub hub) {
LOG.info("Init graph manager");
E.checkArgumentNotNull(conf, "The config can't be null");
+
+ // Auto-generate server.id if not configured.
+ // Random generation is to prevent duplicate id error reports.This id is currently
+ // meaningless and needs to be completely removed serverInfoManager in
+ // the future
Review Comment:
Grammatical error in comment. Should be "Random generation is to prevent duplicate ID error reports. This ID is currently meaningless and needs to be completely removed. serverInfoManager needs to be removed in the future."
```suggestion
// Random generation is to prevent duplicate ID error reports. This ID is currently
// meaningless and needs to be completely removed. serverInfoManager needs to be
// removed in the future.
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1637,10 +1653,6 @@ private void checkBackendVersionOrExit(HugeConfig config) {
private void initNodeRole() {
String id = config.get(ServerOptions.SERVER_ID);
String role = config.get(ServerOptions.SERVER_ROLE);
- E.checkArgument(StringUtils.isNotEmpty(id),
- "The server name can't be null or empty");
- E.checkArgument(StringUtils.isNotEmpty(role),
- "The server role can't be null or empty");
NodeRole nodeRole = NodeRole.valueOf(role.toUpperCase());
Review Comment:
The SERVER_ROLE configuration is still being used at line 1657 with NodeRole.valueOf(role.toUpperCase()), but validation for this value was removed at lines 1656-1659 in the diff (the removed E.checkArgument calls). If the role is empty or invalid, this will throw an IllegalArgumentException from valueOf() without a clear error message. Consider either: (1) keeping the validation if SERVER_ROLE is still needed, or (2) providing a default value handling before calling valueOf() to prevent unclear exceptions.
```suggestion
E.checkArgument(!Strings.isNullOrEmpty(role),
"The server role can't be null or empty, valid values " +
"are %s", Arrays.asList(NodeRole.values()));
NodeRole nodeRole;
try {
nodeRole = NodeRole.valueOf(role.toUpperCase());
} catch (IllegalArgumentException e) {
throw new HugeException(String.format(
"Invalid server role '%s', valid values are %s",
role, Arrays.asList(NodeRole.values())), e);
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -253,6 +261,10 @@ public <V> Future<?> schedule(HugeTask<V> task) {
return this.ephemeralTaskExecutor.submit(task);
}
+ // Validate task state before saving to ensure correct exception type
+ E.checkState(task.type() != null, "Task type can't be null");
+ E.checkState(task.name() != null, "Task name can't be null");
+
Review Comment:
These validation checks are redundant. The same validation is already performed in HugeTask.asArrayWithoutResult() at lines 583-584, which is called later in the save() method. StandardTaskScheduler doesn't have these checks at the schedule() level and relies on the asArrayWithoutResult() validation. Consider removing these duplicate checks to maintain consistency between the two scheduler implementations, or document why DistributedTaskScheduler needs earlier validation.
```suggestion
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1960,7 +1972,7 @@ public HugeGraph graph(String graphSpace, String name) {
} else if (graph instanceof HugeGraph) {
return (HugeGraph) graph;
}
- throw new NotSupportException("graph instance of %s", graph.getClass());
+ throw new NotFoundException(String.format("Graph '%s' does not exist", name));
Review Comment:
The exception type change from NotSupportException to NotFoundException may not accurately reflect the failure condition. This code path is reached when the graph variable is neither null nor an instance of HugeGraph, which suggests a type mismatch (unsupported graph implementation) rather than a missing graph. The original NotSupportException was more semantically correct for this case. Consider reverting to NotSupportException or adding logic to distinguish between "graph not found" and "unsupported graph type" scenarios.
```suggestion
if (graph == null) {
throw new NotFoundException(String.format(
"Graph '%s' does not exist", name));
}
throw new NotSupportException(String.format(
"Graph '%s' is not a HugeGraph instance",
name));
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
- } else {
- return this.deleteFromDB(id);
+ // Already in DELETING status, delete directly from DB
+ // Completed tasks can also be deleted directly
}
+
+ // Delete from DB directly for completed/DELETING tasks or force=true
+ return this.deleteFromDB(id);
}
Review Comment:
The method delete calls taskWithoutResult which will throw NotFoundException if the task doesn't exist (as per the change in TaskAndResultScheduler.java at line 223-225). This means attempting to delete a non-existent task will throw an exception instead of handling it gracefully. Consider adding a try-catch block or checking if the task exists before calling taskWithoutResult, so that deleting a non-existent task is handled appropriately.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3323507991
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -76,20 +77,22 @@ public void testTask() throws TimeoutException {
Assert.assertEquals(id, task.id());
Assert.assertFalse(task.completed());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- scheduler.delete(id, false);
- }, e -> {
- Assert.assertContains("Can't delete incomplete task '88888'",
- e.getMessage());
- });
+ if (scheduler.getClass().equals(StandardTaskScheduler.class)) {
Review Comment:
This PR changes the high-risk `DistributedTaskScheduler` paths for `cancel()`, `delete()`, `close()`, and restore behavior, but the updated tests mostly skip distributed-specific assertions or only relax status expectations. I could not find a test that actually exercises the new distributed `delete(false)` / running-task cancellation paths, which is where the DB-delete-versus-callback-save race above lives. Please add distributed scheduler coverage for at least running task + `delete(false)`, running task + `cancel()`, and close/cron shutdown behavior.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3432918294
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
Review Comment:
I think this is still the key correctness issue in the current HStore distributed path.
The impact is not only that `delete(id, false)` races with cancellation, but that the user-visible delete invariant can be broken:
1. `delete(false)` marks the task as `DELETING`.
2. The cron path sees `DELETING`, calls `cancel(true)`, then immediately removes the task vertex from DB.
3. `HugeTask.cancel()` triggers `TaskCallable.cancelled()`.
4. `UserJob.cancelled()` / `SysJob.cancelled()` call `save()`.
5. `done()` may also persist the final task state.
So a task can be physically deleted and then written back as `CANCELLED` during cancellation cleanup. From the API user perspective, the task was deleted, but it may reappear later.
A minimal reliable fix would be to make delete a two-phase operation for running tasks:
- `delete(false)` only records the delete intent (`DELETING`) and returns the task.
- The runner that actually owns the task cancels it.
- Physical `deleteFromDB()` happens only after the runner has fully exited and no further task callback can save the task again.
One compact way to implement this is to keep an in-memory `deletingTasks` set in `DistributedTaskScheduler`:
```java
private final Set<Id> deletingTasks = ConcurrentHashMap.newKeySet();
```
Then:
- add the id to `deletingTasks` when `delete(false)` transitions the task to `DELETING`;
- in the `DELETING` cron branch, cancel the local running task but do not delete immediately;
- in `TaskRunner.finally`, after `task.run()` has returned and `runningTasks.remove()` is done, delete the task if its id is in `deletingTasks`.
That keeps the fix local to the distributed scheduler and avoids reintroducing master-worker/server-info logic.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
Review Comment:
This is still visible on the current head, and the API impact is clearer now:
`TaskAPI.delete()` treats a `null` return value as "There is no task with id ...", but in this branch `DistributedTaskScheduler.delete(id, false)` returns `null` after successfully marking an existing incomplete task as `DELETING`.
So the request can both:
- mutate the existing task to `DELETING`, and
- report to the caller that the task does not exist.
That makes retry/automation behavior confusing because clients cannot tell whether the delete request was accepted or whether the id was invalid.
The smallest fix is to return the task when the `DELETING` transition succeeds. A slightly safer version should also avoid unconditional `prestatus=null`, so we do not overwrite a concurrently completed task.
```suggestion
HugeTask<V> task = this.taskWithoutResult(id);
if (!force && !task.completed()) {
TaskStatus currentStatus = task.status();
if (this.updateStatus(id, currentStatus, TaskStatus.DELETING)) {
task.overwriteStatus(TaskStatus.DELETING);
} else {
task = this.taskWithoutResult(id);
}
return task;
}
// Delete from DB directly for completed/DELETING tasks or force=true
return this.deleteFromDB(id);
```
This keeps the existing async-delete model, but makes the API contract honest: an existing task marked for deletion is returned as existing, not as missing.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3432920320
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/ServerInfoManager.java:
##########
@@ -77,18 +48,16 @@ public ServerInfoManager(HugeGraphParams graph, ExecutorService dbExecutor) {
this.globalNodeInfo = null;
- this.onlySingleNode = false;
this.closed = false;
}
public void init() {
- HugeServerInfo.schema(this.graph).initSchemaIfNeeded();
+ // ServerInfo is soft-disabled; keep this method for compatibility.
}
public synchronized boolean close() {
Review Comment:
⚠️ `ServerInfoManager` is now soft-disabled, but `close()` still opens the system transaction via `this.tx().close()` and calls `graph.closeTx()`.
That means both scheduler close paths still retain a shutdown-time backend dependency through this compatibility stub, even though server-info persistence is no longer used:
- `StandardTaskScheduler.close()` calls `serverManager.close()`
- `DistributedTaskScheduler.close()` calls `serverManager().close()`
The impact is mostly during shutdown: if the backend connection is already closing, or the graph is partially torn down, scheduler shutdown can still fail/block because a disabled ServerInfo component touches `systemTransaction()`.
The smallest fix is to make `close()` match the new soft-disabled behavior and keep it purely in-memory:
```suggestion
public synchronized boolean close() {
this.closed = true;
return true;
}
```
It would also be good to add a unit test similar to `testInitDoesNotAccessBackendStore()` that verifies `close()` does not call `systemTransaction()` or `closeTx()`.
Related existing comments that should not be missed:
- Running-task delete can resurrect the task record after physical deletion: https://github.com/apache/hugegraph/pull/2937#discussion_r3432918294
- `delete(false)` can mark an existing task as `DELETING` while `TaskAPI.delete()` reports it as missing: https://github.com/apache/hugegraph/pull/2937#discussion_r3432919036
--
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(server): disable server-role in StandardTaskScheduler [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#issuecomment-3734521515
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?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 `55.00000%` with `9 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 29.38%. Comparing base ([`37be6cd`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/37be6cdde3a2ffb71091453b366f28ae446db882?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`47ec466`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/47ec466d13202d2b030c16642f2fbc85bb1c1700?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/incubator-hugegraph/pull/2937?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/apache/hugegraph/task/StandardTaskScheduler.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Ftask%2FStandardTaskScheduler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL3Rhc2svU3RhbmRhcmRUYXNrU2NoZWR1bGVyLmphdmE=) | 66.66% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/hugegraph/task/ServerInfoManager.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Ftask%2FServerInfoManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL3Rhc2svU2VydmVySW5mb01hbmFnZXIuamF2YQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...n/java/org/apache/hugegraph/core/GraphManager.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fcore%2FGraphManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29yZS9HcmFwaE1hbmFnZXIuamF2YQ==) | 75.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...n/java/org/apache/hugegraph/StandardHugeGraph.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2FStandardHugeGraph.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL1N0YW5kYXJkSHVnZUdyYXBoLmphdmE=) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (37be6cd) and HEAD (47ec466). Click for more details.
>
> <details><summary>HEAD has 3 uploads less than BASE</summary>
>
>| Flag | BASE (37be6cd) | HEAD (47ec466) |
>|------|------|------|
>||4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2937 +/- ##
=============================================
- Coverage 39.38% 29.38% -10.01%
+ Complexity 456 375 -81
=============================================
Files 812 801 -11
Lines 68660 67311 -1349
Branches 8968 8724 -244
=============================================
- Hits 27044 19780 -7264
- Misses 38824 45325 +6501
+ Partials 2792 2206 -586
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2937?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.
</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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2696666170
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/StandardTaskScheduler.java:
##########
@@ -739,9 +573,7 @@ public <V> V call(Callable<V> callable) {
}
Review Comment:
next pr do 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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3432920320
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/ServerInfoManager.java:
##########
@@ -77,18 +48,16 @@ public ServerInfoManager(HugeGraphParams graph, ExecutorService dbExecutor) {
this.globalNodeInfo = null;
- this.onlySingleNode = false;
this.closed = false;
}
public void init() {
- HugeServerInfo.schema(this.graph).initSchemaIfNeeded();
+ // ServerInfo is soft-disabled; keep this method for compatibility.
}
public synchronized boolean close() {
Review Comment:
⚠️ `ServerInfoManager` is now soft-disabled, but `close()` still opens the system transaction via `this.tx().close()` and calls `graph.closeTx()`.
That means both scheduler close paths still retain a shutdown-time backend dependency through this compatibility stub, even though server-info persistence is no longer used:
- `StandardTaskScheduler.close()` calls `serverManager.close()`
- `DistributedTaskScheduler.close()` calls `serverManager().close()`
The impact is mostly during shutdown: if the backend connection is already closing, or the graph is partially torn down, scheduler shutdown can still fail/block because a disabled ServerInfo component touches `systemTransaction()`.
The smallest fix is to make `close()` match the new soft-disabled behavior and keep it purely in-memory:
```suggestion
public synchronized boolean close() {
this.closed = true;
return true;
}
```
It would also be good to add a unit test similar to `testInitDoesNotAccessBackendStore()` that verifies `close()` does not call `systemTransaction()` or `closeTx()`.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3323507986
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
Review Comment:
Deleting a running task can race with the task's own cancellation callbacks and resurrect the task record. In this branch the scheduler cancels the in-memory task and immediately removes the task vertex from DB, but `HugeTask.cancel()` invokes `TaskCallable.cancelled()`, and both `UserJob.cancelled()` / `SysJob.cancelled()` call `save()`. Since `taskDone()` is now a no-op, a long-running job deleted via `delete(id, false)` can be removed here and then written back as `CANCELLED` during cancellation cleanup. Please make deletion win over later task saves, or defer physical deletion until the runner has fully stopped and can no longer persist the task.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -76,20 +77,22 @@ public void testTask() throws TimeoutException {
Assert.assertEquals(id, task.id());
Assert.assertFalse(task.completed());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- scheduler.delete(id, false);
- }, e -> {
- Assert.assertContains("Can't delete incomplete task '88888'",
- e.getMessage());
- });
+ if (scheduler.getClass().equals(StandardTaskScheduler.class)) {
Review Comment:
This PR changes the high-risk `DistributedTaskScheduler` paths for `cancel()`, `delete()`, `close()`, and restore behavior, but the updated tests mostly skip distributed-specific assertions or only relax status expectations. I could not find a test that actually exercises the new distributed `delete(false)` / running-task cancellation paths, which is where the DB-delete-versus-callback-save race above lives. Please add distributed scheduler coverage for at least running task + `delete(false)`, running task + `cancel()`, and close/cron shutdown behavior.
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#issuecomment-3758368354
## 代码审查总结
感谢提交这个重要的架构简化 PR!我已经详细审查了所有变更,除了已经在具体代码行发布的评论外,还有以下**关键问题**需要关注:
---
### ‼️ **高优先级问题**
#### 1. Server ID 自动生成机制存在缺陷
在 `GraphManager` 构造函数中:
```java
server = "server-" + UUID.randomUUID().toString().substring(0, 8);
```
**问题**:
- 每次重启会生成新的 ID,导致数据库残留旧的 ServerInfo 记录
- 截取 UUID 前 8 位增加碰撞风险
- 生成的 ID 未持久化,难以追踪和调试
**建议**: 使用 `hostname + processId` 生成稳定 ID,或将 UUID 长度增加到 16 位。
---
#### 2. 调度器类型推断逻辑过于简化
在 `StandardHugeGraph.schedulerType()`:
```java
return StandardHugeGraph.this.isHstore() ? "distributed" : "local";
```
**问题**:
- 仅根据 backend 类型判断,无法覆盖 RocksDB 分布式部署场景
- 未检查 `pd.peers` 等分布式配置
**建议**: 结合 PD 配置或增加显式配置项来判断调度模式。
---
#### 3. 任务删除逻辑发生重大变更
`DistributedTaskScheduler.delete()` 移除了 DELETING 状态的设置逻辑:
**原逻辑**: `force=false` 设置为 DELETING 状态,由定时任务清理
**新逻辑**: `force=false` 时直接删除完成的任务
**影响**: 可能破坏依赖定时清理的代码路径,需在 PR 描述中明确说明此行为变更。
---
### ⚠️ **中等优先级问题**
#### 4. 线程安全隐患
`DistributedTaskScheduler.cancel()` 方法缺少对 `runningTasks` 并发访问的同步保护,可能导致任务在检查和取消之间状态变化。
---
#### 5. 异常处理不完整
`TaskAndResultScheduler.taskWithoutResult()` 新增了 `NotFoundException`,但调用方未统一处理该异常,可能导致不一致的错误行为。
---
#### 6. Ephemeral Job 行为变更未说明
测试用例 `testEphemeralJob()` 的修改表明 Ephemeral Job 不再持久化到数据库,这是重大的 API 行为变更,需要:
- 在 PR 描述中明确说明
- 更新用户文档
- 提供迁移指南
---
#### 7. 资源关闭顺序问题
`DistributedTaskScheduler.close()` 中捕获了 `InterruptedException` 但未恢复线程中断状态:
```java
} catch (ExecutionException | InterruptedException e) {
LOG.warn("Exception while waiting for cron task to complete", e);
// 应该添加: if (e instanceof InterruptedException) Thread.currentThread().interrupt();
}
```
---
### 🧹 **代码质量建议**
1. **TODO 注释应更明确**: 第 334 行的 TODO 缺少问题编号和时间节点
2. **测试用例修改需要说明**: `MultiGraphsTest.testCreateGraphsWithSameName()` 修改图名称的原因未说明
3. **代码覆盖率下降**: Codecov 报告显示 `DistributedTaskScheduler` 新增代码覆盖率仅 0%
---
### 📋 **建议的后续行动**
1. 为 Server ID 生成添加稳定性保证机制
2. 改进调度器类型推断逻辑,支持更多部署场景
3. 明确 API 行为变更并更新文档
4. 增加新增代码路径的单元测试覆盖
5. 考虑添加配置迁移指南
总体而言,这是一个很好的架构简化尝试,但需要解决上述关键问题以确保生产环境的稳定性和可维护性。
--
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(server): fix the scheduler and the scheduler selection logic [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/incubator-hugegraph/pull/2937#discussion_r2804295905
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -253,6 +261,10 @@ public <V> Future<?> schedule(HugeTask<V> task) {
return this.ephemeralTaskExecutor.submit(task);
}
+ // Validate task state before saving to ensure correct exception type
+ E.checkState(task.type() != null, "Task type can't be null");
+ E.checkState(task.name() != null, "Task name can't be null");
+
// Process schema task
// Handle gremlin task
// Handle OLAP calculation tasks
Review Comment:
⚠️ **并发安全问题: 缺少 volatile 或同步保护**
在 `DistributedTaskScheduler.java:268-270`:
```java
if (this.closed.get()) {
return;
}
```
**问题**:
虽然添加了 closed 检查,但 `this.graph.started()` 和 `this.graph.closed()` 在后续调用时可能已经改变,存在 TOCTOU (Time-of-check to time-of-use) 问题。
**建议**:
```suggestion
if (this.closed.get() || !this.graph.started() || this.graph.closed()) {
return;
}
```
将所有状态检查合并到一个 if 语句中,减少竞态窗口。
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
- } else {
- return this.deleteFromDB(id);
+ // Already in DELETING status, delete directly from DB
+ // Completed tasks can also be deleted directly
Review Comment:
‼️ **任务删除逻辑错误**
在 `DistributedTaskScheduler.java:355-366`:
```java
if (!force && !task.completed()) {
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
// 下面的代码永远无法执行
}
```
**问题**:
1. Line 360 的注释 "Already in DELETING status" 放在了 `return null` 之后,永远无法到达
2. 逻辑不清晰:应该区分「未完成的任务」和「已经是 DELETING 状态的任务」
**建议**:
```suggestion
HugeTask<?> task = this.taskWithoutResult(id);
if (!force && !task.completed()) {
// Non-force mode: mark incomplete tasks as DELETING for async cleanup
this.updateStatus(id, null, TaskStatus.DELETING);
return null;
}
// Force mode OR task is completed OR already DELETING: delete directly
return this.deleteFromDB(id);
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -353,6 +396,18 @@ public boolean close() {
cronFuture.cancel(false);
}
+ // Wait for cron task to complete to ensure all transactions are closed
+ try {
+ cronFuture.get(schedulePeriod + 5, TimeUnit.SECONDS);
+ } catch (CancellationException e) {
+ // Task was cancelled, this is expected
+ LOG.debug("Cron task was cancelled");
+ } catch (TimeoutException e) {
+ LOG.warn("Cron task did not complete in time when closing scheduler");
+ } catch (ExecutionException | InterruptedException e) {
+ LOG.warn("Exception while waiting for cron task to complete", e);
+ }
+
Review Comment:
⚠️ **等待超时可能导致数据库事务泄漏**
在 `DistributedTaskScheduler.java:399-410`:
```java
cronFuture.get(schedulePeriod + 5, TimeUnit.SECONDS);
```
**问题**:
1. 如果 cron 任务超时,后续的 `taskDbExecutor` 清理可能在 cron 任务仍持有事务时执行
2. 注释说 "ensure all transactions are closed",但超时时无法保证
3. `schedulePeriod` 可能很大(如 60 秒),导致关闭等待过长
**建议**:
```suggestion
// Wait for cron task with reasonable timeout
long waitTime = Math.min(schedulePeriod + 5, 15); // Cap at 15 seconds
try {
cronFuture.get(waitTime, TimeUnit.SECONDS);
} catch (CancellationException e) {
LOG.debug("Cron task was cancelled");
} catch (TimeoutException e) {
LOG.warn("Cron task did not complete in {}s, proceeding with shutdown", waitTime);
// Force interrupt the cron task thread if possible
cronFuture.cancel(true);
} catch (ExecutionException | InterruptedException e) {
LOG.warn("Exception while waiting for cron task to complete", e);
}
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -363,7 +418,10 @@ public boolean close() {
this.graph.closeTx();
});
}
- return true;
+
+ //todo: serverInfoManager section should be removed in the future.
+ return this.serverManager().close();
+ //return true;
Review Comment:
🧹 **TODO 注释应该追踪到 Issue**
在 `DistributedTaskScheduler.java:422-424`:
```java
//todo: serverInfoManager section should be removed in the future.
return this.serverManager().close();
//return true;
```
**建议**:
将 TODO 关联到 GitHub Issue,方便追踪:
```suggestion
// TODO(#issue-number): Remove serverInfoManager completely after full single-node migration
return this.serverManager().close();
```
并删除注释掉的代码 `//return true;`
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -284,14 +296,41 @@ protected <V> void initTaskParams(HugeTask<V> task) {
}
}
+ /**
+ * Note: This method will update the status of the input task.
+ *
+ * @param task
+ * @param <V>
+ */
@Override
public <V> void cancel(HugeTask<V> task) {
- // Update status to CANCELLING
- if (!task.completed()) {
- // Task not completed, can only execute status not CANCELLING
- this.updateStatus(task.id(), null, TaskStatus.CANCELLING);
+ E.checkArgumentNotNull(task, "Task can't be null");
+
+ if (task.completed() || task.cancelling()) {
+ return;
+ }
+
+ LOG.info("Cancel task '{}' in status {}", task.id(), task.status());
+
+ // Check if task is running locally, cancel it directly if so
+ HugeTask<?> runningTask = this.runningTasks.get(task.id());
+ if (runningTask != null) {
+ boolean cancelled = runningTask.cancel(true);
+ if (cancelled) {
+ task.overwriteStatus(TaskStatus.CANCELLED);
+ }
+ LOG.info("Cancel local running task '{}' result: {}", task.id(), cancelled);
+ return;
+ }
+
+ // Task not running locally, update status to CANCELLING
+ // for cronSchedule() or other nodes to handle
+ TaskStatus currentStatus = task.status();
+ if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
+ LOG.info("Failed to cancel task '{}', status may have changed from {}",
Review Comment:
‼️ **状态更新失败后的任务状态不一致**
在 `DistributedTaskScheduler.java:323-330`:
```java
if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
LOG.info("Failed to cancel task '{}', status may have changed from {}",
task.id(), currentStatus);
} else {
task.overwriteStatus(TaskStatus.CANCELLING);
}
```
**问题**:
- 当 `updateStatus` 返回 false 时(数据库更新失败),内存中的 `task` 对象状态未同步
- 调用者可能认为 cancel 成功,但数据库中状态未改变
- 缺少错误处理:应该重新读取最新状态或抛出异常
**建议**:
```suggestion
TaskStatus currentStatus = task.status();
if (!this.updateStatus(task.id(), currentStatus, TaskStatus.CANCELLING)) {
// Status changed concurrently, reload from DB
HugeTask<?> latestTask = this.taskWithoutResult(task.id());
LOG.info("Failed to cancel task '{}': status changed from {} to {}",
task.id(), currentStatus, latestTask.status());
task.overwriteStatus(latestTask.status());
} else {
task.overwriteStatus(TaskStatus.CANCELLING);
}
```
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3323505377
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -1629,7 +1629,9 @@ public <T> void submitEphemeralJob(EphemeralJob<T> job) {
@Override
public String schedulerType() {
- return StandardHugeGraph.this.schedulerType;
+ // Use distributed scheduler for hstore backend, otherwise use local
+ // After the merger of rocksdb and hstore, consider whether to change this logic
+ return StandardHugeGraph.this.isHstore() ? "distributed" : "local";
Review Comment:
‼️ This changes scheduler selection from an explicit config to a backend-only rule, so an existing non-HStore deployment that set `task.scheduler_type=distributed` will silently fall back to `local` after upgrade. `HugeConfig` only warns for an unregistered option, and `TaskManager.addScheduler()` still trusts only `graph.schedulerType()` here, so there is no migration guard or fail-fast path. Please either keep the old option as an override during migration, or fail fast when the removed option is still configured, and add a compatibility regression test for that upgrade path.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -76,20 +77,22 @@ public void testTask() throws TimeoutException {
Assert.assertEquals(id, task.id());
Assert.assertFalse(task.completed());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- scheduler.delete(id, false);
- }, e -> {
- Assert.assertContains("Can't delete incomplete task '88888'",
- e.getMessage());
- });
+ if (scheduler.getClass().equals(StandardTaskScheduler.class)) {
Review Comment:
⚠️ This guard removes the incomplete-task delete contract coverage for `DistributedTaskScheduler`, even though this PR changes `DistributedTaskScheduler.delete()`. Please add an explicit distributed case instead of skipping it: what should `delete(id, false)` do for a running task, what should happen for a completed task, and what final state should be persisted. Otherwise the test no longer proves the changed distributed delete semantics.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/TaskCoreTest.java:
##########
@@ -629,46 +642,51 @@ public void testGremlinJobAndRestore() throws Exception {
scheduler.cancel(task);
task = scheduler.task(task.id());
- Assert.assertEquals(TaskStatus.CANCELLING, task.status());
+ Assert.assertTrue("Task status should be CANCELLING or CANCELLED, but was " + task.status(),
+ task.status() == TaskStatus.CANCELLING ||
+ task.status() == TaskStatus.CANCELLED);
task = scheduler.waitUntilTaskCompleted(task.id(), 10);
Assert.assertEquals(TaskStatus.CANCELLED, task.status());
Assert.assertTrue("progress=" + task.progress(),
0 < task.progress() && task.progress() < 10);
Assert.assertEquals(0, task.retries());
- Assert.assertEquals(null, task.result());
+ Assert.assertNull(task.result());
HugeTask<Object> finalTask = task;
- Assert.assertThrows(IllegalArgumentException.class, () -> {
- Whitebox.invoke(scheduler.getClass(), "restore", scheduler,
- finalTask);
- }, e -> {
- Assert.assertContains("No need to restore completed task",
- e.getMessage());
- });
- HugeTask<Object> task2 = scheduler.task(task.id());
- Assert.assertThrows(IllegalArgumentException.class, () -> {
+ // because Distributed do nothing in restore, so only test StandardTaskScheduler here
Review Comment:
⚠️ This skips the distributed restore path while the same PR changes distributed cancel/close behavior and relaxes the intermediate cancel assertion to `CANCELLING || CANCELLED`. That leaves the most sensitive distributed state transitions unpinned. Please add distributed-specific regression coverage for the observable cancel state, the final persisted state, and the restore/close behavior instead of excluding the scheduler here.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3432920320
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/ServerInfoManager.java:
##########
@@ -77,18 +48,16 @@ public ServerInfoManager(HugeGraphParams graph, ExecutorService dbExecutor) {
this.globalNodeInfo = null;
- this.onlySingleNode = false;
this.closed = false;
}
public void init() {
- HugeServerInfo.schema(this.graph).initSchemaIfNeeded();
+ // ServerInfo is soft-disabled; keep this method for compatibility.
}
public synchronized boolean close() {
Review Comment:
⚠️ `ServerInfoManager` is now soft-disabled, but `close()` still opens the system transaction via `this.tx().close()` and calls `graph.closeTx()`.
That means both scheduler close paths still retain a shutdown-time backend dependency through this compatibility stub, even though server-info persistence is no longer used:
- `StandardTaskScheduler.close()` calls `serverManager.close()`
- `DistributedTaskScheduler.close()` calls `serverManager().close()`
The impact is mostly during shutdown: if the backend connection is already closing, or the graph is partially torn down, scheduler shutdown can still fail/block because a disabled ServerInfo component touches `systemTransaction()`.
The smallest fix is to make `close()` match the new soft-disabled behavior and keep it purely in-memory:
```suggestion
public synchronized boolean close() {
this.closed = true;
return true;
}
```
It would also be good to add a unit test similar to `testInitDoesNotAccessBackendStore()` that verifies `close()` does not call `systemTransaction()` or `closeTx()`.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#issuecomment-4589382479
Non-blocking review comment: `loadGraph()` now injects server-level `pd.peers` into every graph config when the graph config does not contain it. Please guard this with a non-empty/PD-enabled check, or add coverage for standalone RocksDB graphs, so a default or empty server-level PD setting does not silently change graph startup behavior.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3333317810
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskManager.java:
##########
@@ -102,11 +97,6 @@ private TaskManager(int pool) {
// For a schedule task to run, just one thread is ok
this.schedulerExecutor = ExecutorUtil.newPausableScheduledThreadPool(
1, TASK_SCHEDULER);
- // Start after 10x period time waiting for HugeGraphServer startup
- this.schedulerExecutor.scheduleWithFixedDelay(this::scheduleOrExecuteJob,
- 10 * SCHEDULE_PERIOD,
- SCHEDULE_PERIOD,
- TimeUnit.MILLISECONDS);
}
Review Comment:
TaskManager no longer schedules any periodic work on schedulerExecutor (the scheduleWithFixedDelay() call was removed), but the schedulerExecutor thread pool is still created and kept alive until shutdown. This leaves an idle thread around for no functional benefit and adds extra shutdown/tx-close complexity; consider removing schedulerExecutor (and its related close/shutdown code) if it is no longer part of the scheduling architecture.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
Review Comment:
DistributedTaskScheduler.delete() returns null for existing but incomplete tasks (force=false). This makes TaskAPI.delete() throw "There is no task…" even though the task exists, because TaskAPI only checks for null. Consider returning the task after marking it DELETING (or throw a clear exception) to avoid falsely reporting missing tasks.
--
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(server): fix the scheduler and the scheduler selection logic [hugegraph]
Posted by "Tsukilc (via GitHub)" <gi...@apache.org>.
Tsukilc commented on PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#issuecomment-4892002121
Replaced by #3082 because the source branch has been moved to `hugegraph/hugegraph:fix/scheduler`, so org collaborators can push follow-up fixes directly. Closing this fork-backed PR to avoid duplicate CI/review threads.
--
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