grpc / grpc-go

The Go language implementation of gRPC. HTTP/2 based RPC
https://grpc.io
Apache License 2.0
20.87k stars 4.33k forks source link

pickfirst: Don't use internal.ShuffleAddressListForTesting directly #7515

Open arjan-bal opened 4 weeks ago

arjan-bal commented 4 weeks ago

Based on the PR review comment https://github.com/grpc/grpc-go/pull/7498#discussion_r1718899646,

So, for someone reading the code, it can be a little misleading as to why pickfirst is getting the shuffle func from a var with a ForTesting suffix.

The suggestion from the comment is to instead do something like:

package pickfirst

var shuffleFunc func(n int, swap func(i, j int))

func init() {
  shuffleFunc = rand.Shuffle
  internal.ShuffleAddressListForTesting = func(sf func(n int, swap func(i, j int))) {
    shuffleFunc = sf
  }  
}

func (...) UpdateClientConnState(...) {
  // Directly use `shuffleFunc` to do the shuffling
}

Note that I've changed the signature of internal.ShuffleAddressListForTesting to accept a func. We could rename it to something like internal.PickfirstShuffleFunc. Also, note that we could completely move this var to the pickfirst package and have our tests directly use that, but would pollute the external API.

arjan-bal commented 4 weeks ago

Waiting for for https://github.com/grpc/grpc-go/pull/7498 to get merged so that I can make the change in both pickfirsts together, avoiding merge conflicts.