Initializing Client with Multiple Brokers

[eapache]

This has come up in the back of my mind a few times but I've always held off because I'm not sure what sane default behaviours are in all cases. Right now you specify a single host:port to construct a client and it pulls metadata on the rest of the cluster from that. It makes sense to be able to specify a list of brokers, so that if part of the cluster is offline the client can still connect.

[eapache]

Or does it make more sense for just the caller to loop creating clients until one works - then it can decide its own error behaviour...

[burke]

I'd probably shoot for something like this:

func NewClient(id string, brokers ...string) (*Client, error) {
  for _, broker := range(brokers) {
    host, portstr := strings.SplitN(broker, ":", 2)
    port := strconv.Atoi(portstr)
    // the retry logic you described above
  }
}

So the effective change is that NewClient is invoked as NewClient("my-client", "localhost:8341") instead of with the host and port separate, and becomes variadic. Does that sound reasonable? I can whip up a PR if you're okay with that approach.

[eapache]

This is making me think of another potential issue: if the client cannot connect to all of the listed brokers in Client.update(), it just errors out. This potentially leaks broker connections and isn't necessarily the right thing to do.

This is a surprisingly hard problem :/

[eapache]

As I mentioned in #4, it probably makes sense for the Client, Producer and Consumer constructors to take some sort of Config structs so we can add parameters in a backwards-compatible way. In that case I'm not sure if NewClient(id string, config *ClientConfig, brokers ...string) or NewClient(id string, brokers []string, config *ClientConfig) makes more sense. I like the order of the latter, but would prefer not to have to wrap everything in a slice...

[eapache]

Additional concern: as per the comment near the end of NewClient, broker metadata requests do not seem required to return all the brokers they know about (it is not documented which ones they do return, but I suspect it is the only the subset that leads a partition whose data is also in the response). This means that even once we connect to a broker and retrieve metadata, we should not discard the other n brokers that the user has provided us with - we may need them later if they were not returned in the metadata and all of the ones that were returned go away.

Secondly, if the user gives us two addresses, the first of which is down, we should not block for two minutes (the TCP connection timeout) waiting for the first one before we try the second - we should launch async connections to all of them and use the first that works (or return an error if none of them work).