Copilot commented on code in PR #8102:
URL: https://github.com/apache/incubator-seata/pull/8102#discussion_r3247670566
##########
core/src/main/java/org/apache/seata/core/rpc/netty/ChannelManager.java:
##########
@@ -361,6 +366,113 @@ private static Channel
getChannelFromSameClientMap(Map<Integer, RpcContext> clie
* @return Corresponding channel, NULL if not found.
*/
public static Channel getChannel(String resourceId, String clientId,
boolean tryOtherApp) {
+ return getChannel(resourceId, clientId, tryOtherApp, null, null);
+ }
+
+ /**
+ * Gets channel with transaction context for server-side load balancing.
+ *
+ * Only AT and TCC branch types support server-side load balancing.
+ * XA is excluded because its second-phase operations are bound to the
local database connection
+ * of the original RM. SAGA is excluded because its state machine
execution context is held
+ * in memory with no distributed lock protection. For XA/SAGA and
unconfigured AT/TCC,
+ * the original priority-based channel selection logic is used.
+ *
+ * @param resourceId Resource ID
+ * @param clientId Client ID - ApplicationId:IP:Port
+ * @param tryOtherApp try other app
+ * @param xid global transaction xid, used for load balancing
affinity
+ * @param branchType branch type, determines whether and which LB
algorithm to use
+ * @return Corresponding channel, NULL if not found.
Review Comment:
The Javadoc and signature imply `xid` is used for load-balancing affinity,
but `xid` is not used anywhere in this method (selection only depends on
`candidates`). This is misleading for API consumers; either implement an
affinity strategy that uses `xid` (e.g., consistent hash) or update/remove the
`xid` parameter/Javadoc to reflect the current behavior (reserved for future
use).
##########
core/src/test/java/org/apache/seata/core/rpc/netty/loadbalance/ServerLoadBalanceFactoryTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.core.rpc.netty.loadbalance;
+
+import org.apache.seata.common.loader.EnhancedServiceLoader;
+import org.apache.seata.core.model.BranchType;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Test for ServerLoadBalanceFactory.
+ */
+public class ServerLoadBalanceFactoryTest {
+
+ @Test
+ public void testXaReturnsNull() {
+ // XA does not support server-side load balancing
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.XA);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testSagaReturnsNull() {
+ // SAGA does not support server-side load balancing
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.SAGA);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testAtNotConfiguredReturnsNull() {
+ // When no type is configured, should return null (use original logic)
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.AT);
+ Assertions.assertNull(loadBalance);
+ }
Review Comment:
This test assumes `server.loadBalance.at.type` is not configured in the
environment; if a developer/CI runs tests with this config set
(sysprop/env/file), the assertion will fail. To make the unit test
deterministic, explicitly clear/set
`ServerLoadBalanceFactory.SERVER_LB_AT_TYPE` to blank/null via
`ConfigurationFactory.getInstance().putConfig(...)` (and restore in a
finally/afterEach).
##########
core/src/test/java/org/apache/seata/core/rpc/netty/loadbalance/ServerLoadBalanceFactoryTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.core.rpc.netty.loadbalance;
+
+import org.apache.seata.common.loader.EnhancedServiceLoader;
+import org.apache.seata.core.model.BranchType;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Test for ServerLoadBalanceFactory.
+ */
+public class ServerLoadBalanceFactoryTest {
+
+ @Test
+ public void testXaReturnsNull() {
+ // XA does not support server-side load balancing
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.XA);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testSagaReturnsNull() {
+ // SAGA does not support server-side load balancing
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.SAGA);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testAtNotConfiguredReturnsNull() {
+ // When no type is configured, should return null (use original logic)
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.AT);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testTccNotConfiguredReturnsNull() {
+ ServerLoadBalance loadBalance =
ServerLoadBalanceFactory.getInstance(BranchType.TCC);
+ Assertions.assertNull(loadBalance);
+ }
+
+ @Test
+ public void testSpiLoadRandomLoadBalance() {
+ ServerLoadBalance loadBalance =
EnhancedServiceLoader.load(ServerLoadBalance.class, "RandomLoadBalance");
+ Assertions.assertNotNull(loadBalance);
+ Assertions.assertTrue(loadBalance instanceof ServerRandomLoadBalance);
+ }
+
+ @Test
+ public void testSpiLoadRoundRobinLoadBalance() {
+ ServerLoadBalance loadBalance =
EnhancedServiceLoader.load(ServerLoadBalance.class, "RoundRobinLoadBalance");
+ Assertions.assertNotNull(loadBalance);
+ Assertions.assertTrue(loadBalance instanceof
ServerRoundRobinLoadBalance);
+ }
+
+ @Test
+ public void testSpiLoadLeastActiveLoadBalance() {
+ ServerLoadBalance loadBalance =
EnhancedServiceLoader.load(ServerLoadBalance.class, "LeastActiveLoadBalance");
+ Assertions.assertNotNull(loadBalance);
+ Assertions.assertTrue(loadBalance instanceof
ServerLeastActiveLoadBalance);
+ }
+
+ @Test
+ public void testSpiLoadInvalidTypeThrowsException() {
+ // Loading a non-existent LB type should throw
EnhancedServiceNotFoundException
+ Assertions.assertThrows(
+ Exception.class, () ->
EnhancedServiceLoader.load(ServerLoadBalance.class, "NonExistentLoadBalance"));
+ }
Review Comment:
`testSpiLoadInvalidTypeThrowsException` currently asserts `Exception.class`,
which can hide unexpected failures unrelated to SPI lookup. Prefer asserting
the specific exception type thrown by `EnhancedServiceLoader.load` (e.g.,
`EnhancedServiceNotFoundException`), and consider adding a separate test that
sets an invalid `server.loadBalance.*.type` and verifies
`ServerLoadBalanceFactory.getInstance(...)` returns null (graceful fallback)
rather than propagating.
--
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]
