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