You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by gheo21 <gi...@git.apache.org> on 2016/08/21 14:19:28 UTC
[GitHub] flink pull request #2397: [FLINK-4439] Validate 'bootstrap.servers' config i...
GitHub user gheo21 opened a pull request:
https://github.com/apache/flink/pull/2397
[FLINK-4439] Validate 'bootstrap.servers' config in flink kafka consu\u2026
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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gheo21/flink flink-4439-kafka-consumer-conf-validation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2397.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2397
----
commit 81bfe72f0de2e21941143cd92c3d031962e6bdc7
Author: George <ne...@yahoo.com>
Date: 2016-08-21T14:08:02Z
[FLINK-4439] Validate 'bootstrap.servers' config in flink kafka consumer 0.8
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2397: [FLINK-4439] Validate 'bootstrap.servers' config i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2397
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2397
@gheo21 Do you plan to update this Pull Request?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
done, green build! https://travis-ci.org/apache/flink/builds/157038347
thx!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
@StephanEwen can you please take a look if it makes sense? there is also a small discussion in the Jira issue about it. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2397
Could you rebase to master again, so that the build turns green?
https://travis-ci.org/apache/flink/builds/155979197
Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
@StephanEwen pull request updated, please have a look!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
Hi @rmetzger ,
The build got green. Everything should be ok, right?
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
@StephanEwen I will submit shortly an update then you can take a look. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
Hi @rmetzger,
Thanks for the update, np.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2397
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
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 plan to merge it any time soon?
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2397
+1 to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
Sure, did it. Let's see if it get's green! Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2397
I've rebased to current master and triggered a build. https://travis-ci.org/rmetzger/flink/builds/162871029
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2397
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2397
Thank you. The pull request is now good to be merged!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2397: [FLINK-4439] Validate 'bootstrap.servers' config in flink...
Posted by gheo21 <gi...@git.apache.org>.
Github user gheo21 commented on the issue:
https://github.com/apache/flink/pull/2397
Hi @rmetzger,
Thanks for taking a look. Do I need to do anything else to get it merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---