Don't swallow errors when first connecting.

[rduplain]

Without this change, an exception in the kafka connection results in just Unable to connect to a broker to fetch metadata. without any information about the error. An alternative to this would be to pass the exception object to the log.exception call, but re-raising the exception puts the user code in control.

[yungchin]

It looks like there's another small change in behaviour with this: if you fail to connect to a provided seed host, this no longer tries the next host (if more than one was provided), but excepts immediately. I'm not sure if that's a real objection: if you provide a bad host and a good one, you might as well hear about the bad one. But if we want to keep the old behaviour, something with a for ... else might do?

In any case, I'm with you in preferring to put user code in control here.

[yungchin]

And one second thought, I'm also really bikeshedding here. This makes sense as-is.

[rduplain]

I don't think that's bike-shedding; you are raising an important point. Is the goal of _get_metadata to talk to a broker or all brokers. The patch here is the latter, but it looks like the client need only talk to one broker, which means that the error should go into log.exception so that the programmer/user has feedback. It would help to have a specific exception, rather than Exception.

[yungchin]

How about re-raising only after you fall off the end of the loop? That way you'd still inform user code directly.

[rduplain]

How about re-raising only after you fall off the end of the loop? That way you'd still inform user code directly.

What should we do if we have multiple exceptions, i.e. different exceptions from different brokers?

[yungchin]

I wouldn't know a really correct solution for that, but I suppose raising only the last one that occurred should be ok? If the user code deals with that problem and then retries, it should work because it can talk to at least the last broker.

And I guess the exceptions can still also be logged, so that if you did inspect the logs you could see all of them?

[rduplain]

I propose passing each error to log.exception, then raising a RuntimeError instead of Exception.

[yungchin]

Sounds good to me.

[rduplain]

I propose passing each error to log.exception, then raising a RuntimeError instead of Exception.

The result of this proposal is less than ideal in my usecase. I'm connecting via streamparse (this might be an issue for that project), and the log message ends up in the logs directory instead of in the console, where a pykafka client is set in the initialization of a component.

[rduplain]

I pushed a patch. This is ready if we all agree to the proposal.

[emmettbutler]

Looks good to me. It's correct that this function should continue trying brokers until one succeeds or it's tried all of them.