Copilot commented on code in PR #7878:
URL: https://github.com/apache/incubator-seata/pull/7878#discussion_r2637776176
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -182,6 +167,21 @@ public Result<String> createGroup(String namespace, String
vGroup, String cluste
public Result<String> createGroup(
String namespace, String vGroup, String clusterName, String
unitName, boolean checkExist) {
+ // If unitName is blank, find it from cluster
+ String actualUnitName = unitName;
+ if (StringUtils.isBlank(unitName)) {
+ Map<String, ClusterData> clusterDataMap =
namespaceClusterDataMap.get(namespace);
+ if (clusterDataMap != null) {
+ ClusterData clusterData = clusterDataMap.get(clusterName);
+ if (clusterData != null &&
!CollectionUtils.isEmpty(clusterData.getUnitData())) {
+ Optional<Map.Entry<String, Unit>> optionalEntry =
+
clusterData.getUnitData().entrySet().stream().findFirst();
+ if (optionalEntry.isPresent()) {
+ actualUnitName = optionalEntry.get().getKey();
+ }
+ }
+ }
+ }
Review Comment:
When unitName is blank and the cluster has no units available,
actualUnitName remains blank. This could cause issues when calling the HTTP
endpoint if the backend expects a non-empty unit parameter. Consider adding a
check to return an error result when actualUnitName is still blank after the
fallback logic, or handle this case explicitly.
```suggestion
}
if (StringUtils.isBlank(actualUnitName)) {
LOGGER.error("no available unit for namespace {} and cluster
{}", namespace, clusterName);
return new Result<>("400", "no available unit for cluster: " +
clusterName);
}
```
##########
console/src/main/resources/static/console-fe/src/reducers/locale.ts:
##########
@@ -25,7 +25,7 @@ const zhCN = Object.assign({}, fusionZhCN, I18N.zhCN);
interface LocaleStateModel {
language: string;
- locale: ILocale;
+ locale: any;
Review Comment:
Changing locale type from ILocale to any weakens type safety. This removes
compile-time checks and autocomplete support for locale properties. Consider
creating a proper interface or type definition for the locale structure instead
of using any.
##########
console/src/main/resources/static/console-fe/src/service/clusterManager.ts:
##########
@@ -31,3 +31,9 @@ export async function fetchClusterData(namespace: string,
clusterName: string):
});
return result;
}
+
+export async function changeGroup(namespace: string, clusterName: string,
vGroup: string): Promise<any> {
+ const params: any = { namespace, clusterName, vGroup };
+ const result = await request.post('/naming/changeGroup', params);
+ return result;
Review Comment:
The changeGroup function in clusterManager.ts doesn't include headers with
Content-Type for form-urlencoded data like addGroup does. The backend expects
form-urlencoded data based on the transactionInfo.ts implementation. This
inconsistency may cause the request to fail or behave unexpectedly. Ensure the
changeGroup function uses the same format as addGroup with qs.stringify and
appropriate headers.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -474,6 +474,11 @@ public void instanceHeartBeatCheck() {
public Result<String> changeGroup(String namespace, String vGroup, String
clusterName, String unitName) {
long changeTime = System.currentTimeMillis();
ConcurrentMap<String, NamespaceBO> namespaceMap = new
ConcurrentHashMap<>(vGroupMap.get(vGroup));
+ Result<String> res = createGroup(namespace, vGroup, clusterName,
unitName, false);
+ if (!res.isSuccess()) {
+ LOGGER.error("add vgroup failed!" + res.getMessage());
Review Comment:
The error log message concatenates the result message without proper
spacing. Consider using string formatting or adding a space between "add vgroup
failed!" and the message to improve log readability.
```suggestion
LOGGER.error("add vgroup failed! {}", res.getMessage());
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -539,17 +540,50 @@ public SingleResult<Map<String,
org.apache.seata.namingserver.entity.vo.v2.Names
// Build NamespaceVOv2
Map<String, org.apache.seata.namingserver.entity.vo.v2.NamespaceVO>
namespaceVOs = new HashMap<>();
- data.clustersMap.forEach((namespace, clusters) -> {
+ data.getClustersMap().forEach((namespace, clusters) -> {
org.apache.seata.namingserver.entity.vo.v2.NamespaceVO namespaceVO
=
new
org.apache.seata.namingserver.entity.vo.v2.NamespaceVO();
- Map<String, List<String>> clusterVgList = new HashMap<>();
- Map<String, Set<String>> clusterVgSet =
data.clusterVgroupsMap.get(namespace);
+ Map<String, org.apache.seata.namingserver.entity.vo.v2.ClusterVO>
clusterVOMap = new HashMap<>();
+
clusters.forEach(cluster -> {
- Set<String> vgSet = clusterVgSet.get(cluster);
- clusterVgList.put(cluster, vgSet != null ? new
ArrayList<>(vgSet) : new ArrayList<>());
+ org.apache.seata.namingserver.entity.vo.v2.ClusterVO clusterVO
=
+ new
org.apache.seata.namingserver.entity.vo.v2.ClusterVO();
+
+ // Set units and type for this cluster
+ Map<String, ClusterData> clusterDataMap =
namespaceClusterDataMap.get(namespace);
+ String clusterType = "default";
+ List<String> unitNames = new ArrayList<>();
+ if (clusterDataMap != null) {
+ ClusterData clusterData = clusterDataMap.get(cluster);
+ if (clusterData != null) {
+ unitNames = clusterData.getUnitData().values().stream()
+ .map(Unit::getUnitName)
+ .collect(Collectors.toList());
+ clusterType = clusterData.getClusterType();
+ clusterVO.setType(clusterType);
+ }
+ }
+ clusterVO.setUnits(unitNames);
+
+ // Set vgroups based on cluster type
+ if ("RAFT".equalsIgnoreCase(clusterType)) {
+ // For RAFT clusters, collect all unique vgroups
+ Map<String, Set<String>> clusterVgSet =
+ data.getClusterVgroupsMap().get(namespace);
+ Set<String> vgSet = clusterVgSet.get(cluster);
+ clusterVO.setVgroups(vgSet != null ? new
ArrayList<>(vgSet) : new ArrayList<>());
+ } else {
+ // For non-RAFT clusters, set vgroups directly
+ Map<String, Set<String>> clusterVgSet =
+ data.getClusterVgroupsMap().get(namespace);
+ Set<String> vgSet = clusterVgSet.get(cluster);
+ clusterVO.setVgroups(vgSet != null ? new
ArrayList<>(vgSet) : new ArrayList<>());
+ }
Review Comment:
The if-else branches for RAFT and non-RAFT clusters contain identical logic.
Both branches retrieve vgroups from the clusterVgroupsMap and set them on the
ClusterVO. Consider removing the duplicate logic and consolidating into a
single code block since there's no actual difference in behavior between the
cluster types here.
```suggestion
// Set vgroups (same logic for all cluster types)
Map<String, Set<String>> clusterVgSet =
data.getClusterVgroupsMap().get(namespace);
Set<String> vgSet = clusterVgSet != null ?
clusterVgSet.get(cluster) : null;
clusterVO.setVgroups(vgSet != null ? new ArrayList<>(vgSet)
: new ArrayList<>());
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/entity/vo/v2/ClusterVO.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.namingserver.entity.vo.v2;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Value Object representing cluster information for the v2 API.
+ * <p>
+ * This class encapsulates the information for a single cluster, including
+ * its associated vgroups, units, and cluster type.
+ */
+public class ClusterVO {
+
+ private List<String> vgroups = new ArrayList<>();
+
+ private Map<String, List<String>> unitVgroups = new HashMap<>();
Review Comment:
The ClusterVO has a unitVgroups field (Map<String,
List<String>>) that is initialized but never populated or used. This
field should either be populated with the unit-to-vgroups mapping data or
removed if not needed.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -515,17 +516,17 @@ public SingleResult<Map<String, NamespaceVO>> namespace()
{
NamespaceData data = collectNamespaceData();
Map<String, NamespaceVO> namespaceVOs = new HashMap<>();
- if (data.vgroupsMap.isEmpty()) {
- data.clustersMap.forEach((namespace, clusters) -> {
+ if (data.getVgroupsMap().isEmpty()) {
+ data.getClustersMap().forEach((namespace, clusters) -> {
NamespaceVO namespaceVO = new NamespaceVO();
namespaceVO.setClusters(new ArrayList<>(clusters));
namespaceVOs.put(namespace, namespaceVO);
});
} else {
- data.vgroupsMap.forEach((namespace, vgroups) -> {
+ data.getVgroupsMap().forEach((namespace, vgroups) -> {
NamespaceVO namespaceVO =
namespaceVOs.computeIfAbsent(namespace, k -> new NamespaceVO());
- Set<String> clusters = data.clustersMap.get(namespace);
- namespaceVO.setClusters(new ArrayList<>(clusters != null ?
clusters : Collections.emptyList()));
+ Set<String> clusters = data.getClustersMap().get(namespace);
+ namespaceVO.setClusters(new ArrayList<>(clusters != null ?
clusters : Collections.<String>emptyList()));
Review Comment:
Using explicit type parameter Collections.<String>emptyList() is
unnecessary in Java 8+. The type can be inferred as Collections.emptyList()
since the context already provides the type information from the ArrayList
constructor.
```suggestion
namespaceVO.setClusters(new ArrayList<>(clusters != null ?
clusters : Collections.emptyList()));
```
##########
console/src/main/resources/static/console-fe/src/service/transactionInfo.ts:
##########
@@ -248,28 +249,18 @@ export async function startBranchData(params:
BranchSessionParam): Promise<any>
return result;
}
-export async function addGroup(namespace: string, clusterName: string, vGroup:
string, unitName?: string): Promise<any> {
- let result = await request('/naming/addGroup', {
- method: 'POST',
- params: {
- namespace,
- clusterName,
- vGroup,
- unitName,
- },
+export async function addGroup(namespace: string, clusterName: string, vGroup:
string, unitName: string = ''): Promise<any> {
Review Comment:
The function signature has changed from optional unitName parameter
(unitName?: string) to required with default empty string (unitName: string =
''). However, the changeGroup function in clusterManager.ts doesn't pass a
unitName parameter at all. This will result in the default empty string being
used, which may not be the intended behavior for RAFT clusters. Consider
ensuring all call sites pass the appropriate unitName when needed.
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -1303,14 +1371,36 @@ class TransactionInfo extends
React.Component<GlobalProps, TransactionInfoState>
hasClear
placeholder={selectTargetClusterPlaceholder}
onChange={(value: string) => {
- this.setState({ targetCluster: value });
+ this.setState(prevState => {
+ const namespaceData =
prevState.namespaceOptions.get(prevState.targetNamespace);
+ const clusterUnits = namespaceData ?
namespaceData.clusterUnits : {};
+ const units = clusterUnits[value] || [];
+ return {
+ targetCluster: value,
+ targetUnits: units,
+ targetUnit: units.length > 0 ? units[0] : '',
+ };
+ });
}}
dataSource={this.state.targetClusters.map(value => ({ label:
value, value }))}
value={this.state.targetCluster}
/>
</FormItem>
+ {this.state.targetCluster &&
this.state.namespaceOptions.get(this.state.targetNamespace)?.clusterTypes[this.state.targetCluster]
!== 'default' && (
+ <FormItem name="targetUnit" label="targetUnit">
+ <Select
+ hasClear
+ placeholder="Select target unit"
+ onChange={(value: string) => {
+ this.setState({ targetUnit: value });
+ }}
+ dataSource={this.state.targetUnits.map(value => ({ label:
value, value }))}
+ value={this.state.targetUnit}
+ />
+ </FormItem>
+ )}
<FormItem>
- <Button type="primary" onClick={this.handleChangeVGroup}
disabled={!this.state.selectedVGroup || !this.state.targetNamespace ||
!this.state.targetCluster}>
+ <Button type="primary" onClick={this.handleChangeVGroup}
disabled={!this.state.selectedVGroup || !this.state.targetNamespace ||
!this.state.targetCluster ||
(this.state.namespaceOptions.get(this.state.targetNamespace)?.clusterTypes[this.state.targetCluster]
!== 'default' && !this.state.targetUnit)}>
Review Comment:
The button's disabled condition is complex and difficult to maintain.
Consider extracting this logic into a computed variable or method with a
descriptive name to improve readability and maintainability.
##########
console/src/main/resources/static/console-fe/src/pages/ClusterManager/ClusterManager.tsx:
##########
@@ -215,11 +223,14 @@ class ClusterManager extends React.Component<GlobalProps,
ClusterManagerState> {
};
render() {
- const { locale = {} } = this.props;
+ const { locale } = this.props;
const rawLocale = locale.ClusterManager;
const clusterManagerLocale: ClusterManagerLocale = typeof rawLocale ===
'object' && rawLocale !== null ? rawLocale : {};
const { title, subTitle, selectNamespaceFilerPlaceholder,
selectClusterFilerPlaceholder, searchButtonLabel, unitName, members,
clusterType, view, unitDialogTitle, control, transaction, weight, healthy,
term, unit, operations, internal, version, metadata, controlEndpoint,
transactionEndpoint, metadataDialogTitle } = clusterManagerLocale;
const unitData = this.state.clusterData ?
Object.entries(this.state.clusterData.unitData || {}) : [];
+ const { namespace } = this.state;
+ const namespaceData = namespace ?
this.state.namespaceOptions.get(namespace) : null;
+ const allClusters = namespaceData ? namespaceData.clusters : [];
Review Comment:
Unused variable allClusters.
```suggestion
```
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -1303,14 +1371,36 @@ class TransactionInfo extends
React.Component<GlobalProps, TransactionInfoState>
hasClear
placeholder={selectTargetClusterPlaceholder}
onChange={(value: string) => {
- this.setState({ targetCluster: value });
+ this.setState(prevState => {
+ const namespaceData =
prevState.namespaceOptions.get(prevState.targetNamespace);
+ const clusterUnits = namespaceData ?
namespaceData.clusterUnits : {};
+ const units = clusterUnits[value] || [];
+ return {
+ targetCluster: value,
+ targetUnits: units,
+ targetUnit: units.length > 0 ? units[0] : '',
+ };
+ });
}}
dataSource={this.state.targetClusters.map(value => ({ label:
value, value }))}
value={this.state.targetCluster}
/>
</FormItem>
+ {this.state.targetCluster &&
this.state.namespaceOptions.get(this.state.targetNamespace)?.clusterTypes[this.state.targetCluster]
!== 'default' && (
+ <FormItem name="targetUnit" label="targetUnit">
+ <Select
+ hasClear
+ placeholder="Select target unit"
Review Comment:
The 'targetUnit' label should follow the same naming convention as other
labels in the dialog. Consider using a more descriptive and user-friendly label
like 'Target Unit' to match the pattern of other form labels.
```suggestion
<FormItem name="targetUnit" label="Target Unit">
<Select
hasClear
placeholder="Select Target Unit"
```
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -892,12 +911,19 @@ class TransactionInfo extends
React.Component<GlobalProps, TransactionInfoState>
}
showCreateVGroupDialog = () => {
- this.setState(prevState => ({
- createVGroupDialogVisible: true,
- vGroupName: '',
- createNamespace: prevState.globalSessionParam.namespace || '',
- createCluster: prevState.globalSessionParam.cluster || '',
- }));
+ this.setState(prevState => {
+ const clusters = prevState.globalSessionParam.namespace ?
prevState.namespaceOptions.get(prevState.globalSessionParam.namespace)?.clusters
|| [] : [];
Review Comment:
Unused variable clusters.
```suggestion
```
##########
namingserver/pom.xml:
##########
@@ -276,6 +276,14 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <source>8</source>
+ <target>8</target>
+ </configuration>
+ </plugin>
Review Comment:
The maven-compiler-plugin configuration is redundant. The parent pom already
defines this plugin with the same source/target configuration using properties
maven.compiler.source and maven.compiler.target. This duplication should be
removed to maintain consistency with the project-wide compiler settings defined
in the parent pom.
```suggestion
```
--
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]
