Copilot commented on code in PR #7878:
URL: https://github.com/apache/incubator-seata/pull/7878#discussion_r2637802265
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -182,6 +167,25 @@ 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:
The logic for finding a unit when unitName is blank selects the first
available unit using stream().findFirst(). This is non-deterministic behavior
since the order of entries in a HashMap/ConcurrentHashMap is not guaranteed.
For better predictability and stability, consider using a LinkedHashMap or
sorting the entries before selecting one, or document that the selection is
arbitrary.
```suggestion
Optional<String> optionalUnitName =
clusterData.getUnitData().keySet().stream().sorted().findFirst();
if (optionalUnitName.isPresent()) {
actualUnitName = optionalUnitName.get();
```
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -545,7 +564,7 @@ class TransactionInfo extends React.Component<GlobalProps,
TransactionInfoState>
startGlobalSessionTitle,
sendGlobalSessionTitle,
changeGlobalSessionTitle,
- } = locale;
+ } = locale.TransactionInfo || {};
Review Comment:
The code uses optional chaining and assumes locale is always defined due to
the Redux connection, but in lines 567 and 758, it accesses
locale.TransactionInfo without checking if locale exists. While this is likely
safe with the Redux setup, defensive coding would suggest adding a fallback
(e.g., locale?.TransactionInfo || {}) to prevent potential runtime errors if
the Redux state is not properly initialized.
```suggestion
} = locale?.TransactionInfo || {};
```
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -1290,7 +1364,9 @@ class TransactionInfo extends
React.Component<GlobalProps, TransactionInfoState>
return {
targetNamespace: value,
targetClusters: clusters,
- targetCluster: clusters.length > 0 ? clusters[0] : '',
+ targetCluster: '',
+ targetUnits: [],
+ targetUnit: '',
};
Review Comment:
When targetNamespace is changed, targetCluster is reset to an empty string
(line 1367), but the logic doesn't select a default cluster like it does for
createNamespace (where createCluster is set to clusters[0]). This inconsistency
could lead to user confusion where one dialog pre-selects a cluster and the
other doesn't. Consider setting targetCluster to clusters[0] when
clusters.length > 0 for a consistent user experience.
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -351,14 +356,26 @@ class TransactionInfo extends
React.Component<GlobalProps, TransactionInfoState>
loadNamespaces = async () => {
try {
const namespaces = await fetchNamespaceV2();
- const namespaceOptions = new Map<string, { clusters: string[],
clusterVgroups: {[key: string]: string[]} }>();
+ const namespaceOptions = new Map<string, NamespaceData>();
+
Object.keys(namespaces).forEach(namespaceKey => {
const namespaceData = namespaces[namespaceKey];
- const clusterVgroups = (namespaceData.clusterVgroups || {}) as {[key:
string]: string[]};
- const clusters = Object.keys(clusterVgroups);
+ const clustersData = namespaceData.clusters || {};
+ const clusterVgroups: {[key: string]: string[]} = {};
+ const clusterUnits: {[key: string]: string[]} = {};
+ const clusterTypes: {[key: string]: string} = {};
+ Object.keys(clustersData).forEach(clusterName => {
+ const cluster = clustersData[clusterName];
+ clusterVgroups[clusterName] = cluster.vgroups || [];
+ clusterUnits[clusterName] = cluster.units || [];
+ clusterTypes[clusterName] = cluster.type || 'default';
+ });
+ const clusters = Object.keys(clustersData);
namespaceOptions.set(namespaceKey, {
clusters,
clusterVgroups,
+ clusterUnits,
+ clusterTypes,
});
Review Comment:
The namespace data processing logic is duplicated across three components
(TransactionInfo, GlobalLockInfo, and ClusterManager). All three have nearly
identical code for parsing namespaces, extracting cluster data, and building
the namespaceOptions Map. This logic should be extracted into a shared utility
function to improve maintainability and ensure consistency.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -474,6 +478,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());
+ return res;
+ }
Review Comment:
The order of operations in changeGroup has been reversed - createGroup is
now called before collecting existing namespace/cluster mappings. While this
change may be intentional to ensure the new group exists before removing the
old one, it could lead to a partial state if the subsequent removeGroup
operations fail. Consider adding compensating logic or documentation explaining
why this order is necessary and how failures are handled.
##########
console/src/main/resources/static/console-fe/src/service/clusterManager.ts:
##########
@@ -31,3 +32,11 @@ export async function fetchClusterData(namespace: string,
clusterName: string):
});
return result;
}
+
+export async function changeGroup(namespace: string, clusterName: string,
vGroup: string, unitName: string = ''): Promise<any> {
+ const params = { namespace, clusterName, unitName, vGroup };
+ const result = await request.post('/naming/changeGroup',
qs.stringify(params), {
+ headers: { 'Content-Type': 'application/x-www-form-urlencoded' }
+ });
+ return result;
+}
Review Comment:
This function is duplicated with the same implementation in
transactionInfo.ts (lines 260-266). Consider extracting this common function to
a shared location to avoid code duplication and ensure consistency across the
codebase.
```suggestion
export async function postChangeGroup(
namespace: string,
clusterName: string,
vGroup: string,
unitName: string = '',
): Promise<any> {
const params = { namespace, clusterName, unitName, vGroup };
const result = await request.post('/naming/changeGroup',
qs.stringify(params), {
headers: { 'Content-Type': 'application/x-www-form-urlencoded' }
});
return result;
}
export async function changeGroup(namespace: string, clusterName: string,
vGroup: string, unitName: string = ''): Promise<any> {
return postChangeGroup(namespace, clusterName, vGroup, unitName);
}
```
##########
console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx:
##########
@@ -18,11 +18,11 @@ import React from 'react';
import { ConfigProvider, Table, Button, DatePicker, Form, Icon, Switch,
Pagination, Dialog, Input, Select, Message } from
'@alicloud/console-components';
import Actions, { LinkButton } from '@alicloud/console-components-actions';
import { withRouter } from 'react-router-dom';
+import { connect } from 'react-redux';
import Page from '@/components/Page';
import { GlobalProps } from '@/module';
import getData, { changeGlobalData, deleteBranchData, deleteGlobalData,
GlobalSessionParam, sendGlobalCommitOrRollback,
startBranchData, startGlobalData, stopBranchData, stopGlobalData,
forceDeleteGlobalData, forceDeleteBranchData, fetchNamespaceV2, addGroup,
changeGroup } from '@/service/transactionInfo';
-import PropTypes from 'prop-types';
import moment from 'moment';
Review Comment:
The PropTypes import and static propTypes declaration have been removed but
the import is still present in ClusterManager.tsx, GlobalLockInfo.tsx, and
Login.tsx. This creates an inconsistency across the codebase. Either remove
PropTypes from all components or keep it in all for consistency, especially
since TypeScript provides static type checking through the GlobalProps
interface.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -587,14 +623,23 @@ private NamespaceData collectNamespaceData() {
Set<String> vgroups = vgroupsMap.get(namespace);
vgroups.add(vGroup);
- // Build cluster to vgroups
+ // Build cluster to vgroups and unit to vgroups mapping
Map<String, Set<String>> clusterVg =
clusterVgroupsMap.get(namespace);
+ Map<String, Map<String, Set<String>>> clusterUnitVg =
unitVgroupsMap.get(namespace);
+
namespaceBO.getClusterMap().forEach((clusterName, clusterBO) -> {
Set<String> vgSet = clusterVg.computeIfAbsent(clusterName, k
-> new HashSet<>());
vgSet.add(vGroup);
+
+ // For each unit in this cluster, add the vgroup
+ Map<String, Set<String>> unitVgMap =
clusterUnitVg.computeIfAbsent(clusterName, k -> new HashMap<>());
+ clusterBO.getUnitNames().forEach(unitName -> {
+ Set<String> unitVgSet =
unitVgMap.computeIfAbsent(unitName, k -> new HashSet<>());
+ unitVgSet.add(vGroup);
+ });
Review Comment:
The unitVgroupsMap is being collected and passed to NamespaceData
constructor but appears to be unused in the namespaceV2() method. The method
only uses clustersMap, vgroupsMap, and clusterVgroupsMap when building the
response. If unitVgroupsMap is intended for future use or a different API
endpoint, consider documenting this or removing it if it's truly unused to
avoid confusion.
##########
console/src/main/resources/static/console-fe/src/reducers/locale.ts:
##########
@@ -52,7 +52,7 @@ const getCurrentLanguage = (): string => {
return lang;
}
-const getCurrentLocaleObj = (): any => {
+const getCurrentLocaleObj = (): ILocale => {
Review Comment:
The return type has been weakened from ILocale to any, which removes type
safety for locale objects throughout the application. This change should be
avoided to maintain type checking. Consider updating the return type to match
the actual locale structure being used.
##########
console/src/main/resources/static/console-fe/src/pages/ClusterManager/ClusterManager.tsx:
##########
@@ -215,11 +224,13 @@ 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 { title, subTitle, selectNamespaceFilerPlaceholder,
selectClusterFilerPlaceholder, searchButtonLabel, unitName, members,
clusterType, view, unitDialogTitle, control, transaction, weight, healthy,
term, unit, operations, internal, version, metadata, controlEndpoint,
transactionEndpoint, metadataDialogTitle, role } = clusterManagerLocale;
Review Comment:
Unused variable unitName.
```suggestion
const { title, subTitle, selectNamespaceFilerPlaceholder,
selectClusterFilerPlaceholder, searchButtonLabel, members, clusterType, view,
unitDialogTitle, control, transaction, weight, healthy, term, unit, operations,
internal, version, metadata, controlEndpoint, transactionEndpoint,
metadataDialogTitle, role } = clusterManagerLocale;
```
--
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]
