Fix ConnOpenRandom case in ConnOpenStrategy during connection dial
#1655
Conversation
This makes sure the random open strategy always tries all given addresses to find working server eventually. Previously there is a chance, that same non-working server could have been picked twice thus ending up not picking working server at all (by not exploring all given addresses) Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the ConnOpenRandom connection strategy where the same non-working server could be selected multiple times, preventing the discovery of working servers when trying all available addresses.
- Moves random number generation outside the connection loop to ensure all addresses are properly explored
- Removes dependency on manual random seeding in tests, making them deterministic
- Re-enables previously skipped random failover tests
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| clickhouse.go | Fixes random strategy by generating random number once per connection attempt |
| clickhouse_std.go | Applies same fix for standard driver connection logic |
| tests/utils.go | Removes global random seeding utilities and test setup |
| tests/conn_test.go | Re-enables random failover test |
| tests/std/conn_test.go | Re-enables random failover test |
| examples/clickhouse_api/utils.go | Removes random seeding utility functions |
| examples/clickhouse_api/multi_host.go | Removes manual seeding from random example |
| examples/clickhouse_api/main_test.go | Removes random seeding from test setup |
| examples/std/main_test.go | Removes random seeding from test setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Looks like this still uses the original Also I remember seeing this code change from this PR/issue: Let me know what you think. I understand the random strategy has a risk of picking two broken connections, but if the users wanted a real sequence they would use the RoundRobin strategy. It looks like this simply changes the random offset. I would be open to completely removing the random strategy |
|
Thanks @SpencerTorres for more info and context. I see the problem. You are right. Just doing So the actual problem is this "We have to establish N connections using M servers randomly for load balancing, without choosing non-working ones more than once" correct? how about we shuffle the list every time before establishing a connection? so we get both random and no repetition. What do you think? |
2 participants
Fixes: #1653
Summary
This makes sure the random open strategy always tries all given addresses to find working server eventually.
Previously there is a chance, that same non-working server could have been picked twice thus ending up not picking working server at all (by not exploring all given addresses)
Basically, implemented the proposal (1) as mentioned in this comment.
Main changes
rand.Int()outside the loop once. And use it withrand + ito cover all given addresses correctlyseed()in the tests.Now the test passes without any extra maintenance like seeding.
This always passes now without random failure.
Checklist
Delete items not relevant to your PR: