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