You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "paultanay (via GitHub)" <gi...@apache.org> on 2026/09/22 08:30:33 UTC
[PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
paultanay opened a new pull request, #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196
**What this PR does**:
Updates `loadbalance.Select` to explicitly handle and dispatch `ConsistentHashLoadBalance` and
`LeastActiveLoadBalance` strategies. Previously, these values fell through to the default branch
and incorrectly routed to the random load balancing strategy. This PR also introduces a
comprehensive dispatcher-level regression test suite in `pkg/remoting/loadbalance/loadbalance_test.
go` to ensure correctness across all load balancing configurations.
**Which issue(s) this PR fixes**:
Fixes #1073
**Special notes for your reviewer**:
The implementations for `ConsistentHashLoadBalance` and `LeastActiveLoadBalance` were already
present in the codebase but lacked the necessary routing logic in the central `Select` function.
The 7 newly added tests verify proper routing for all 5 strategies, confirm the fallback behavior,
validate empty/closed session handling, and cover concurrent dispatch execution. All 27 tests in
the package pass locally.
**Does this PR introduce a user-facing change?**:
```release-note
NONE
```
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
Posted by "Ethan-Xingyue (via GitHub)" <gi...@apache.org>.
Ethan-Xingyue commented on PR #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196#issuecomment-5788572870
plz fix ci
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
Posted by "thunguo (via GitHub)" <gi...@apache.org>.
thunguo commented on PR #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196#issuecomment-6382985573
Plz resolve the conflicts.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196#issuecomment-5788512986
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1196?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 61.58%. Comparing base ([`0f3ebc2`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/0f3ebc2bea56c12b2a1d290b8d8443ee76e4ce03?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ebbc01d`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/ebbc01df8070d530aa2d87b9eec52f9ad1f2659d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1196 +/- ##
==========================================
+ Coverage 61.55% 61.58% +0.03%
==========================================
Files 309 309
Lines 22625 22629 +4
==========================================
+ Hits 13927 13937 +10
+ Misses 7416 7409 -7
- Partials 1282 1283 +1
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1196?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196#discussion_r4072472307
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
Review Comment:
These tests only assert that `Select(...)` returns non-nil, which would still pass even if `Select` dispatches to the wrong strategy (e.g., falling back to random). To make this a regression suite for dispatcher routing, assert strategy-specific, deterministic outcomes (e.g., unique `RemoteAddr()` per session and expected chosen session for consistent hash / xid; multiple sequential calls for round-robin to verify rotation; for random, constrain to a single eligible session so the outcome is deterministic).
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
+ var wg sync.WaitGroup
+ strategies := []string{
+ randomLoadBalance,
+ xidLoadBalance,
+ roundRobinLoadBalance,
+ consistentHashLoadBalance,
+ leastActiveLoadBalance,
+ }
+
+ for i := 0; i < 100; i++ {
+ wg.Add(1)
+ go func(idx int) {
+ defer wg.Done()
+ strategy := strategies[idx%len(strategies)]
+ conn := Select(strategy, sessions, "test_xid")
+ assert.NotNil(t, conn)
+ }(i)
+ }
Review Comment:
Calling `assert.NotNil(t, ...)` from multiple goroutines is not safe because it writes to the shared `*testing.T`. This can cause data races and flaky failures under `-race`. Collect results/errors from goroutines (e.g., via a channel or atomic flag) and assert on the main goroutine after `wg.Wait()`.
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
Review Comment:
There are blank lines containing trailing whitespace (tabs/spaces). Please remove trailing whitespace to keep diffs clean and avoid lint/format failures in repositories enforcing whitespace rules.
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
+ var wg sync.WaitGroup
+ strategies := []string{
+ randomLoadBalance,
+ xidLoadBalance,
+ roundRobinLoadBalance,
+ consistentHashLoadBalance,
+ leastActiveLoadBalance,
+ }
+
Review Comment:
There are blank lines containing trailing whitespace (tabs/spaces). Please remove trailing whitespace to keep diffs clean and avoid lint/format failures in repositories enforcing whitespace rules.
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
+ var wg sync.WaitGroup
+ strategies := []string{
+ randomLoadBalance,
+ xidLoadBalance,
+ roundRobinLoadBalance,
+ consistentHashLoadBalance,
+ leastActiveLoadBalance,
+ }
+
+ for i := 0; i < 100; i++ {
+ wg.Add(1)
+ go func(idx int) {
+ defer wg.Done()
+ strategy := strategies[idx%len(strategies)]
+ conn := Select(strategy, sessions, "test_xid")
+ assert.NotNil(t, conn)
+ }(i)
+ }
+
Review Comment:
There are blank lines containing trailing whitespace (tabs/spaces). Please remove trailing whitespace to keep diffs clean and avoid lint/format failures in repositories enforcing whitespace rules.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: dispatch missing load balance strategies in Select (#1073) [incubator-seata-go]
Posted by "Ethan-Xingyue (via GitHub)" <gi...@apache.org>.
Ethan-Xingyue commented on code in PR #1196:
URL: https://github.com/apache/incubator-seata-go/pull/1196#discussion_r4080686683
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
Review Comment:
These tests still pass with the two new `case` lines removed: every mock returns the same `RemoteAddr()` (line 34), `NotNil` holds for any strategy, and without `resetConsistentHashForTest()` the ring can be empty and fall back to random. Please give each mock a distinct address and assert strategy-specific results, e.g. mark all but one address busy with `rpc.BeginCount` and expect that one from `leastActiveLoadBalance` (line 71), and after `resetConsistentHashForTest()` expect repeated `Select(consistentHashLoadBalance, ...)` calls with the same xid to return the same connection (use an xid that doesn't hash past the last virtual node; see the wrap-around note on `loadbalance.go`).
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
Review Comment:
CI `golang ci lint` fails here: lines 95, 104 and 114 contain only a tab, which `gofmt -s` rejects. Running `sh goimports.sh` (or `goimports -local seata.apache.org/seata-go/v2 -w`) fixes them and also moves the `seata.apache.org/seata-go/v2/pkg/remoting/mock` import on line 26 into its own group.
##########
pkg/remoting/loadbalance/loadbalance.go:
##########
@@ -39,6 +39,10 @@ func Select(loadBalanceType string, sessions *sync.Map, xid string) connection.C
return XidLoadBalance(sessions, xid)
case roundRobinLoadBalance:
return RoundRobinLoadBalance(sessions, xid)
+ case consistentHashLoadBalance:
Review Comment:
Not blocking, pre-existing: `ConsistentHashLoadBalance` builds its ring once via a package-level `sync.Once` and rebuilds it only when a picked connection is closed, while both managers pass a fresh snapshot on every call. So if the first call sees no sessions, the ring stays empty and every pick falls back to random; TC nodes that join or reconnect later stay out of the ring until some ringed connection closes. Suggested follow-up: rebuild the ring whenever the snapshot's membership changes, as the Java `ConsistentHashSelectorWrapper` does.
##########
pkg/remoting/loadbalance/loadbalance.go:
##########
@@ -39,6 +39,10 @@ func Select(loadBalanceType string, sessions *sync.Map, xid string) connection.C
return XidLoadBalance(sessions, xid)
case roundRobinLoadBalance:
return RoundRobinLoadBalance(sessions, xid)
+ case consistentHashLoadBalance:
+ return ConsistentHashLoadBalance(sessions, xid)
+ case leastActiveLoadBalance:
+ return LeastActiveLoadBalance(sessions, xid)
Review Comment:
Pre-existing, but only reachable through this case: `LeastActiveLoadBalance` calls `rpc.GetStatus(addr).GetActive()`, which reads `Status.Active` without synchronization while `SendSync`/`SendAsync` update it with `atomic.AddInt32`, so concurrent requests race on it. Please switch `GetActive`/`GetTotal` in `pkg/remoting/rpc/rpc_status.go` to `atomic.LoadInt32`.
##########
pkg/remoting/loadbalance/loadbalance.go:
##########
@@ -39,6 +39,10 @@ func Select(loadBalanceType string, sessions *sync.Map, xid string) connection.C
return XidLoadBalance(sessions, xid)
case roundRobinLoadBalance:
return RoundRobinLoadBalance(sessions, xid)
+ case consistentHashLoadBalance:
Review Comment:
Not blocking, pre-existing: when the key hashes past the last virtual node, `pick` returns `RandomLoadBalance(...)` instead of wrapping to `sortedHashNodes[0]`, so those xids may go to a different TC on each call. Suggested follow-up: wrap around like the Java selector (`tailMap.isEmpty() ? firstKey() : tailMap.firstKey()`).
##########
pkg/remoting/loadbalance/loadbalance.go:
##########
@@ -39,6 +39,10 @@ func Select(loadBalanceType string, sessions *sync.Map, xid string) connection.C
return XidLoadBalance(sessions, xid)
case roundRobinLoadBalance:
return RoundRobinLoadBalance(sessions, xid)
+ case consistentHashLoadBalance:
+ return ConsistentHashLoadBalance(sessions, xid)
Review Comment:
Pre-existing, but this case makes it user-visible: `xid` is a constant here in production. `SendSync`/`SendAsync` pass the whole `message.RpcMessage` to `selectSession`/`selectChannel` (`getty_remoting.go:53,71`, `grpc_remoting.go:52,71`), so `getXid` returns `"<invalid Value>"` (getty) or `""` (gRPC) for every message, and `ConsistentHashLoadBalance` sends every request, branch commit/rollback responses included, to the same TC. Please pass `msg.Body` there and, like the Java client, fall back to a random key when no xid is found, either here or in a PR that lands first.
##########
pkg/remoting/loadbalance/loadbalance_test.go:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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 loadbalance
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/golang/mock/gomock"
+ "github.com/stretchr/testify/assert"
+ "seata.apache.org/seata-go/v2/pkg/remoting/mock"
+)
+
+func setupSessions(ctrl *gomock.Controller, count int, isClosed bool) *sync.Map {
+ sessions := &sync.Map{}
+ for i := 0; i < count; i++ {
+ session := mock.NewMockTestSession(ctrl)
+ session.EXPECT().IsClosed().Return(isClosed).AnyTimes()
+ session.EXPECT().RemoteAddr().Return("127.0.0.1:8080").AnyTimes()
+ sessions.Store(session, "session")
+ }
+ return sessions
+}
+
+func TestSelect_RandomLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_XidLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(xidLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_RoundRobinLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(roundRobinLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_ConsistentHashLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(consistentHashLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_LeastActiveLoadBalance(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select(leastActiveLoadBalance, sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_FallbackToRandom(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, false)
+ conn := Select("UnknownType", sessions, "test_xid")
+ assert.NotNil(t, conn)
+}
+
+func TestSelect_EmptyClosedSessions(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 3, true)
+ conn := Select(randomLoadBalance, sessions, "test_xid")
+ assert.Nil(t, conn)
+
+ emptySessions := &sync.Map{}
+ assert.Nil(t, Select(randomLoadBalance, emptySessions, "test_xid"))
+}
+
+func TestSelect_ConcurrentDispatch(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := setupSessions(ctrl, 10, false)
+
+ var wg sync.WaitGroup
+ strategies := []string{
+ randomLoadBalance,
+ xidLoadBalance,
+ roundRobinLoadBalance,
+ consistentHashLoadBalance,
+ leastActiveLoadBalance,
+ }
+
+ for i := 0; i < 100; i++ {
+ wg.Add(1)
+ go func(idx int) {
+ defer wg.Done()
+ strategy := strategies[idx%len(strategies)]
+ conn := Select(strategy, sessions, "test_xid")
+ assert.NotNil(t, conn)
Review Comment:
Not blocking: `assert.NotNil` from these goroutines is fine (`assert` reports through `t.Errorf`, which is goroutine-safe), but nothing here calls `rpc.BeginCount`/`EndCount` while `LeastActiveLoadBalance` runs, which is the concurrency `SendSync` produces. Adding such a goroutine would cover the `GetActive` race under `-race`.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org