Copilot commented on code in PR #1111:
URL:
https://github.com/apache/incubator-seata-go/pull/1111#discussion_r3109389835
##########
pkg/remoting/loadbalance/loadbalance.go:
##########
@@ -31,14 +31,21 @@ const (
leastActiveLoadBalance = "LeastActiveLoadBalance"
)
-func Select(loadBalanceType string, sessions *sync.Map, xid string)
getty.Session {
+// Select dispatches to the balancer named by loadBalanceType.
+// consistent is only used by the consistent-hash strategy; other strategies
+// ignore it and may safely receive nil.
+func Select(loadBalanceType string, sessions *sync.Map, xid string, consistent
*Consistent) getty.Session {
Review Comment:
Changing the exported `Select` signature is a breaking API change for any
external users of `pkg/remoting/loadbalance`. Consider preserving the old
`Select(loadBalanceType, sessions, xid)` as a wrapper (passing a
default/optional `*Consistent`), and introduce a new function (or variadic
option) for callers that want to supply a ring.
```suggestion
// It preserves the original exported API and uses a nil consistent-hash ring
// unless the caller explicitly uses SelectWithConsistent.
func Select(loadBalanceType string, sessions *sync.Map, xid string)
getty.Session {
return SelectWithConsistent(loadBalanceType, sessions, xid, nil)
}
// SelectWithConsistent dispatches to the balancer named by loadBalanceType.
// consistent is only used by the consistent-hash strategy; other strategies
// ignore it and may safely receive nil.
func SelectWithConsistent(loadBalanceType string, sessions *sync.Map, xid
string, consistent *Consistent) getty.Session {
```
##########
pkg/remoting/loadbalance/consistent_hash_loadbalance_test.go:
##########
@@ -182,3 +177,38 @@ func TestConsistentPick_ConcurrentPickAndRefresh(t
*testing.T) {
assert.NotNil(t, result)
assert.False(t, result.IsClosed())
}
+
+// TestConsistentRefresh_SerializedWriters asserts that two goroutines
+// rebuilding the ring at the same time still leave it in a consistent
+// "every hash-node maps to something in the circle" state.
+func TestConsistentRefresh_SerializedWriters(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ sessions := &sync.Map{}
+
+ for i := 0; i < 4; i++ {
+ addr := fmt.Sprintf("127.0.0.1:70%02d", i)
+ s := mock.NewMockTestSession(ctrl)
+ s.EXPECT().IsClosed().AnyTimes().Return(false)
+ s.EXPECT().RemoteAddr().AnyTimes().Return(addr)
+ sessions.Store(s, addr)
+ }
+
+ c := NewConsistent(0)
+
+ var wg sync.WaitGroup
+ for i := 0; i < 16; i++ {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ c.refreshHashCircle(sessions)
+ }()
+ }
+ wg.Wait()
+
+ c.RLock()
+ defer c.RUnlock()
+ assert.Equal(t, len(c.sortedHashNodes), len(c.hashCircle))
+ for _, pos := range c.sortedHashNodes {
+ assert.NotNil(t, c.hashCircle[pos])
Review Comment:
This test asserts `len(sortedHashNodes) == len(hashCircle)`, but
`sortedHashNodes` can legitimately contain duplicate positions while
`hashCircle` is a map (duplicates overwrite). Rare hash collisions will make
the map smaller than the slice and can cause flaky failures. Prefer removing
the length-equality assertion and only asserting that each `pos` in
`sortedHashNodes` maps to a non-nil entry, and/or deduplicate positions when
building `sortedHashNodes`.
```suggestion
for _, pos := range c.sortedHashNodes {
session, ok := c.hashCircle[pos]
assert.True(t, ok)
assert.NotNil(t, session)
```
##########
pkg/remoting/loadbalance/random_loadbalance.go:
##########
@@ -38,11 +35,8 @@ func RandomLoadBalance(sessions *sync.Map, xid string)
getty.Session {
}
return true
})
- //keys eq 0 means there are no available session
if len(keys) == 0 {
return nil
}
- //random in keys
- randomIndex :=
rand.New(rand.NewSource(time.Now().UnixNano())).Intn(len(keys))
- return keys[randomIndex]
+ return keys[rand.Intn(len(keys))]
Review Comment:
`rand.Intn` uses the global `math/rand` source, but the repo doesn’t seed it
anywhere (no `rand.Seed` found). This changes RandomLoadBalance from
time-seeded randomness to a deterministic sequence across process starts. Seed
`math/rand` once (e.g., in a package init) or use a package-level `rand.Rand`
with an explicit seed.
##########
pkg/remoting/loadbalance/least_active_loadbalance.go:
##########
@@ -56,10 +55,8 @@ func LeastActiveLoadBalance(sessions *sync.Map, xid string)
getty.Session {
if leastCount == 0 {
return nil
}
-
if leastCount == 1 {
return leastIndexes[0]
- } else {
- return
leastIndexes[rand.New(rand.NewSource(time.Now().UnixNano())).Intn(leastCount)]
}
+ return leastIndexes[rand.Intn(leastCount)]
Review Comment:
`rand.Intn` uses the global `math/rand` source, but the repo doesn’t seed it
anywhere (no `rand.Seed` found). This makes tie-breaking among least-active
sessions deterministic across process starts. Seed `math/rand` once (package
init) or use a dedicated `rand.Rand` with an explicit seed.
--
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]
