Copilot commented on code in PR #7891:
URL: https://github.com/apache/incubator-seata/pull/7891#discussion_r2638877541
##########
common/src/main/java/org/apache/seata/common/metadata/namingserver/NamingServerNode.java:
##########
@@ -78,7 +78,9 @@ public boolean isChanged(Object obj) {
NamingServerNode otherNode = (NamingServerNode) obj;
// other node is newer than me
- return otherNode.term > term ||
!StringUtils.equals(otherNode.getVersion(), this.getVersion());
+ return !Objects.equals(this.getRole(), otherNode.getRole())
+ || otherNode.term > term
Review Comment:
The role comparison logic may incorrectly treat older nodes as "changed".
According to the method's intent (detecting if "other node is newer than me"),
a role difference alone should not trigger a change detection if the other node
has an older or equal term. The current logic `!Objects.equals(this.getRole(),
otherNode.getRole()) || otherNode.term > term` will return true for any role
difference, even when otherNode.term < term. Consider changing the logic to:
`otherNode.term > term || (otherNode.term >= term &&
!Objects.equals(this.getRole(), otherNode.getRole())) ||
!StringUtils.equals(otherNode.getVersion(), this.getVersion())` to ensure role
changes are only considered for nodes with equal or higher terms.
```suggestion
return otherNode.term > term
|| (otherNode.term >= term &&
!Objects.equals(this.getRole(), otherNode.getRole()))
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -192,15 +192,20 @@ public Result<String> createGroup(
return new Result<>("400", "vGroup " + vGroup + " already exists");
}
// add vGroup in new cluster
- List<Node> nodeList = getInstances(namespace, clusterName);
+ List<NamingServerNode> nodeList = getInstances(namespace, clusterName);
if (nodeList == null || nodeList.size() == 0) {
LOGGER.error("no instance in cluster {}", clusterName);
return new Result<>("301", "no instance in cluster:" +
clusterName);
} else {
- Node node = nodeList.stream()
+ List<NamingServerNode> filteredNodes = nodeList.stream()
.filter(n -> n.getRole() == ClusterRole.LEADER ||
n.getRole() == ClusterRole.MEMBER)
- .collect(Collectors.toList())
- .get(0);
+ .sorted((o1, o2) -> Long.compare(o2.getTerm(),
o1.getTerm()))
+ .collect(Collectors.toList());
+ if (filteredNodes.isEmpty()) {
+ LOGGER.error("no suitable instance (LEADER or MEMBER) in
cluster {}", clusterName);
+ return new Result<>("301", "no suitable instance in cluster:"
+ clusterName);
+ }
+ NamingServerNode node = filteredNodes.get(0);
Review Comment:
The logic for selecting a node from the filtered list has changed
significantly. Previously, the code used `.get(0)` directly on the stream
result, which would throw an IndexOutOfBoundsException if the list was empty.
Now there's proper empty list handling with an error check. However, there's a
subtle issue: the sorting by term in descending order (line 202) is good for
selecting the node with the highest term, but this may not always be the best
choice if there are multiple nodes with the same highest term. Consider adding
a secondary sorting criterion (e.g., by role preference LEADER over MEMBER) to
ensure deterministic selection.
--
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]
