Skip to content

Conversation

@kavirajk
Copy link
Contributor

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

  1. Create rand.Int() outside the loop once. And use it with rand + i to cover all given addresses correctly
  2. Remove the need for any seed() in the tests.

Now the test passes without any extra maintenance like seeding.

go test ./tests/... -run TestConnFailoverRandom -count=1 -v

This always passes now without random failure.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

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]>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SpencerTorres
Copy link
Member

Looks like this still uses the original math/rand package, so it wouldn't fully close #1653

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

@kavirajk
Copy link
Contributor Author

kavirajk commented Sep 16, 2025

Thanks @SpencerTorres for more info and context. I see the problem. You are right. Just doing rand.Int() still maintains the order (not truly random)

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to math/rand/v2

2 participants