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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
