Copilot commented on code in PR #7586:
URL: https://github.com/apache/incubator-seata/pull/7586#discussion_r2374023132
##########
common/src/main/java/org/apache/seata/common/metadata/Node.java:
##########
@@ -99,6 +100,32 @@ public void setVersion(String version) {
this.version = version;
}
+ @JsonIgnore
+ public boolean isHttp2Supported() {
+ String baseVersion = "2.6.0";
+
+ if (version == null || version.isEmpty()) {
+ return false;
+ }
+
+ String[] current = version.split("\\.");
+ String[] base = baseVersion.split("\\.");
+
+ int len = Math.max(current.length, base.length);
+ for (int i = 0; i < len; i++) {
+ int cur = i < current.length ? Integer.parseInt(current[i]) : 0;
+ int bas = i < base.length ? Integer.parseInt(base[i]) : 0;
Review Comment:
The parseInt() calls can throw NumberFormatException if version parts
contain non-numeric characters (e.g., '2.6.0-SNAPSHOT'). Add validation or use
a try-catch block to handle malformed version strings gracefully.
```suggestion
int cur = 0;
int bas = 0;
if (i < current.length) {
try {
cur =
Integer.parseInt(current[i].replaceAll("[^0-9].*$", ""));
} catch (NumberFormatException e) {
// Malformed version part, treat as 0
cur = 0;
}
}
if (i < base.length) {
try {
bas = Integer.parseInt(base[i].replaceAll("[^0-9].*$",
""));
} catch (NumberFormatException e) {
bas = 0;
}
}
```
##########
discovery/seata-discovery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java:
##########
@@ -221,46 +223,59 @@ protected static void startQueryMetadata() {
}
}
- private static String queryHttpAddress(String clusterName, String group) {
+ private static Node selectNodeForHttpAddress(String clusterName, String
group) {
List<Node> nodeList = METADATA.getNodes(clusterName, group);
- List<String> addressList = null;
- Stream<InetSocketAddress> stream = null;
+
if (CollectionUtils.isNotEmpty(nodeList)) {
List<InetSocketAddress> inetSocketAddresses =
ALIVE_NODES.get(CURRENT_TRANSACTION_SERVICE_GROUP);
+
if (CollectionUtils.isEmpty(inetSocketAddresses)) {
- addressList = nodeList.stream()
- .map(RaftRegistryServiceImpl::selectControlEndpointStr)
- .collect(Collectors.toList());
- } else {
- stream = inetSocketAddresses.stream();
+ return
nodeList.get(ThreadLocalRandom.current().nextInt(nodeList.size()));
}
- } else {
- stream = INIT_ADDRESSES.get(clusterName).stream();
- }
- if (addressList != null) {
- return
addressList.get(ThreadLocalRandom.current().nextInt(addressList.size()));
- } else {
+
Map<String, Node> map = new HashMap<>();
- if (CollectionUtils.isNotEmpty(nodeList)) {
- for (Node node : nodeList) {
- InetSocketAddress inetSocketAddress =
selectTransactionEndpoint(node);
- map.put(inetSocketAddress.getHostString() +
IP_PORT_SPLIT_CHAR + inetSocketAddress.getPort(), node);
- }
+ for (Node node : nodeList) {
+ InetSocketAddress inetSocketAddress =
selectTransactionEndpoint(node);
+ map.put(inetSocketAddress.getHostString() + IP_PORT_SPLIT_CHAR
+ inetSocketAddress.getPort(), node);
}
- addressList = stream.map(inetSocketAddress -> {
- String host = NetUtil.toStringHost(inetSocketAddress);
- Node node = map.get(host + IP_PORT_SPLIT_CHAR +
inetSocketAddress.getPort());
- InetSocketAddress controlEndpoint = null;
- if (node != null) {
- controlEndpoint = selectControlEndpoint(node);
- }
- return host
- + IP_PORT_SPLIT_CHAR
- + (controlEndpoint != null ?
controlEndpoint.getPort() : inetSocketAddress.getPort());
- })
+
+ List<Node> aliveNodes = inetSocketAddresses.stream()
+ .map(addr -> map.get(NetUtil.toStringHost(addr) +
IP_PORT_SPLIT_CHAR + addr.getPort()))
+ .filter(Objects::nonNull)
.collect(Collectors.toList());
- return
addressList.get(ThreadLocalRandom.current().nextInt(addressList.size()));
+
+ if (!aliveNodes.isEmpty()) {
+ return
aliveNodes.get(ThreadLocalRandom.current().nextInt(aliveNodes.size()));
+ }
+ } else {
+ List<InetSocketAddress> initAddresses =
INIT_ADDRESSES.get(clusterName);
+ if (CollectionUtils.isNotEmpty(initAddresses)) {
+
+ return null;
Review Comment:
This logic is inverted. When initAddresses is not empty, the method should
process them and return a Node, not return null. The return null should be when
initAddresses is empty.
```suggestion
InetSocketAddress inetSocketAddress =
initAddresses.get(ThreadLocalRandom.current().nextInt(initAddresses.size()));
// Construct a Node with the selected address. Other fields
can be set to defaults or null.
Node node = new Node();
node.setAddress(NetUtil.toStringAddress(inetSocketAddress));
node.setCluster(clusterName);
node.setGroup(group);
return node;
```
##########
discovery/seata-discovery-raft/src/test/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImplTest.java:
##########
@@ -116,9 +112,23 @@ public void testRefreshTokenSuccess()
when(HttpClientUtil.doPost(any(String.class), any(Map.class),
any(Map.class), any(int.class)))
.thenReturn(mockResponse);
- Method refreshTokenMethod =
RaftRegistryServiceImpl.class.getDeclaredMethod("refreshToken", String.class);
+ Method refreshTokenMethod =
+
RaftRegistryServiceImpl.class.getDeclaredMethod("refreshToken", String.class,
Node.class);
refreshTokenMethod.setAccessible(true);
- refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(),
"127.0.0.1:8092");
+
+ Node mockNode = mock(Node.class);
+ Map<String, Object> metadata = new HashMap<>();
+ List<Map<String, Object>> externalEndpoints = new ArrayList<>();
+ LinkedHashMap<String, Object> externalEndpoint = new
LinkedHashMap<>();
+ externalEndpoint.put("host", "10.10.105.7");
+ externalEndpoint.put("controlPort", 30071);
+ externalEndpoint.put("transactionPort", 30091);
+ externalEndpoints.add(externalEndpoint);
+ metadata.put("external", externalEndpoints);
+ when(mockNode.getMetadata()).thenReturn(metadata);
+
+ refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode);
Review Comment:
Same issue as above - the method should be invoked on
RaftRegistryServiceImpl.getInstance() instead of null since refreshToken is not
a static method.
```suggestion
refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(),
"127.0.0.1:8092", mockNode);
```
##########
discovery/seata-discovery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java:
##########
@@ -506,7 +635,7 @@ private static void acquireClusterMetaData(String
clusterName, String group) thr
response =
EntityUtils.toString(httpResponse.getEntity(), StandardCharsets.UTF_8);
} else if (statusCode == HttpStatus.SC_UNAUTHORIZED) {
if (StringUtils.isNotBlank(USERNAME) &&
StringUtils.isNotBlank(PASSWORD)) {
- refreshToken(tcAddress);
+ refreshToken(tcAddress, selectedNode);
Review Comment:
The refreshToken method signature has been updated to accept (String
clusterName, Node selectedNode), but here it's being called with (String
tcAddress, Node selectedNode). The first parameter should be the clusterName,
not tcAddress.
```suggestion
refreshToken(clusterName, selectedNode);
```
##########
discovery/seata-discovery-raft/src/test/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImplTest.java:
##########
@@ -84,18 +83,15 @@ public void testLoginFailed() throws IOException,
NoSuchMethodException {
when(HttpClientUtil.doPost(any(String.class), any(Map.class),
any(Map.class), any(int.class)))
.thenReturn(mockResponse);
- // Use reflection to access and invoke the private method
- Method refreshTokenMethod =
RaftRegistryServiceImpl.class.getDeclaredMethod("refreshToken", String.class);
+ Method refreshTokenMethod =
+
RaftRegistryServiceImpl.class.getDeclaredMethod("refreshToken", String.class,
Node.class);
refreshTokenMethod.setAccessible(true);
- assertThrows(
- Exception.class,
- () ->
refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(),
"127.0.0.1:8092"));
+
+ Node mockNode = mock(Node.class);
+ assertThrows(Exception.class, () ->
refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode));
Review Comment:
The method is being invoked with null as the instance parameter, but
refreshToken is not a static method. It should be invoked on
RaftRegistryServiceImpl.getInstance() instead of null.
```suggestion
assertThrows(Exception.class, () ->
refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(),
"127.0.0.1:8092", mockNode));
```
--
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]
