Shuffle seed brokers so we don't always connect to the first one provided #441
Conversation
| @@ -720,3 +723,11 @@ func (client *client) getConsumerMetadata(consumerGroup string, attemptsRemainin | |||
| client.resurrectDeadBrokers() | |||
| return retry(ErrOutOfBrokers) | |||
| } | |||
|
|
|||
| // shuffleSeedBrokers performs a Fisher-Yates shuffle on the seed broker list | |||
There was a problem hiding this comment.
Simpler just to use rand.Perm()?
|
Just cast to |
|
Yup, that's what I am doing right now, but I have to do that after the client is initialized. |
| for _, addr := range addrs { | ||
| client.seedBrokers = append(client.seedBrokers, NewBroker(addr)) | ||
|
|
||
| for _, index := range rand.Perm(len(addrs)) { |
There was a problem hiding this comment.
we need to seed the random number generator; to avoid destroying our parent program (if it has already seeded with a specific value for some reason) we shouldn't use the global but instantiate a local source instead... not sure if it makes sense to create a package-local in sarama.go or just a real local right here
There was a problem hiding this comment.
I've added a package-local sarama.Random, which I seed using time.Now().UnixNano().
|
Did you intend for this to be in 1.4 or not? |
|
I think this would be a nice addition to 1.4. It's such a minimal change that it doesn't require an extra batch of stress testing. |
| @@ -22,6 +24,8 @@ import ( | |||
| // but you can set it to redirect wherever you want. | |||
| var Logger StdLogger = log.New(ioutil.Discard, "[Sarama] ", log.LstdFlags) | |||
|
|
|||
| var Random = rand.New(rand.NewSource(time.Now().UnixNano())) | |||
There was a problem hiding this comment.
Should this really be public? I'm not sure why anyone would want to override it. If you want to leave it public, it needs godoc
| addr3 := seed3.Addr() | ||
|
|
||
| // Overwrite the seed brokers with a fixed ordering to make this test deterministic. | ||
| client.seedBrokers = []*Broker{NewBroker(addr1), NewBroker(addr2), NewBroker(addr3)} |
There was a problem hiding this comment.
rather than completely overriding it, just append the two new ones is simpler
There was a problem hiding this comment.
I feel this is simpler to understand, because now you can basically ignore what happened before; the seed broker state is complete defined here. YMMV.
|
This version LGTM. |
|
Looks good. How often is the NewClient called, generating a new permutation? More specifically, how large can the list of brokers be? Generating a permutation often can be expensive, but if the list is small, it should not matter. Shiv |
|
The number of brokers in a cluster may be hundreds for installations that are significantly larger than ours, but especially in those environments I would like to prevent all my clients from connecting to the same broker :) |
|
looks good. |
Shuffle seed brokers so we don't always connect to the first one provided
|
Also, for what it's worth: building the permutation appears to be |
|
Nice.. I should have looked at that before my claims :-) |
I stole this idea from the Zookeeper library because is such a simple yet powerful way to load balance the metadata requests in the cluster over the different brokers. After the initial seed broker shuffle, seed broker handling is deterministic, and will work the exact same way before.
The way I fixed the "resurrect dead brokers" test is not great, but that is primarily because of the limitation of the mock broker implementation. If you have ideas to do this in a better way, tell me :)
@Shopify/kafka