[FLINK-4439] Validate 'bootstrap.servers' config in flink kafka consu…

[gheo21]

Hello everybody,

I would like to contribute a small improvement to Flink.
Lately, I was using the FlinkKafkaConsumer08 to write a streaming topology in flink. Somehow I mistakenly configured the 'boostrap.servers' for the kafka config with invalid hosts.

The message that flink provided was not clearly stating what the problem was. Hence, my improvement consists of a validation of the servers provided in 'boostrap.servers'.

If none of the configured servers are valid then we should fail-fast and a validation exception should be thrown. If at lease one server is valid then we don't throw any exception.

See for more info: https://issues.apache.org/jira/browse/FLINK-4439

[gheo21]

@StephanEwen can you please take a look if it makes sense? there is also a small discussion in the Jira issue about it. Thanks!

[StephanEwen]

The validation code may easily fail if the broker list format is off.
How about only doing extra work in case of a java.nio.channels.ClosedChannelException? You can catch the exception, validate, throw a better exception in that case, or rethrow the original exception.

[StephanEwen]

@gheo21 Do you plan to update this Pull Request?

[gheo21]

Hi @StephanEwen,

Thanks for taking a look at this.
This would only make sense if I catch the exception in the catch block of the getPartitionsForTopic method. However, I can only check the ClosedChannelException with instance of :( because the consumer.send(req); is throwing it as a runtime exception. Would this be ok for you? then I do there the validation as you suggested and throw an illegal argument if all the servers are invalid.

Although this would complicate the exception logic code. If you think that this situation does not occur that often, that the users misconfigure all of the boostrap servers and that the logging info it's enough, we can of course close the merge request. I tried to make an improvement but it's only a suggestion ;)

[gheo21]

@StephanEwen I will submit shortly an update then you can take a look. Thanks!

[gheo21]

@StephanEwen pull request updated, please have a look!

[rmetzger]

+1 to merge

[gheo21]

Hi @rmetzger,
Thanks for taking a look. Do I need to do anything else to get it merged?

[rmetzger]

Could you rebase to master again, so that the build turns green?
https://travis-ci.org/apache/flink/builds/155979197
Thank you!

[gheo21]

Sure, did it. Let's see if it get's green! Thanks.

[gheo21]

done, green build! https://travis-ci.org/apache/flink/builds/157038347
thx!

[gheo21]

Hi @rmetzger ,

The build got green. Everything should be ok, right?

Thanks!

[rmetzger]

Thank you. The pull request is now good to be merged!

[rmetzger]

I've rebased to current master and triggered a build. https://travis-ci.org/rmetzger/flink/builds/162871029

[gheo21]

Hi guys @StephanEwen @rmetzger , I've rebased once again the branch and fixed the merge conflicts. The travis build has two failed builds but one is connection reset by maven and the other one is coming from some incompatible source file which I didn't touch.
https://travis-ci.org/apache/flink/builds/165262138

So how is the merge process? do you still plan to merge it?

Thanks.

[rmetzger]

Yes, I'm still planning to merge it. I was sick the last few weeks, that's why I didn't proceed. I'm hopefully okay now and I'll try to merge your change today.
Thank you for rebasing it again.

[gheo21]

Hi @rmetzger,
Thanks for the update, np.