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