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