[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530:
URL: https://github.com/apache/kafka/pull/10530#issuecomment-825190538
@tombentley and @C0urante Thanks for the above comment.
I peeked at the code for upgrading to Apache HttpClient and this seems like
a big work especially around the SSL factory and other parameters. This has the
potential to cause even bigger disruption. I am a new committer and the
benefits of doing this upgrade don't seem worth it to me as this bug seems like
a very corner case. All we want to do is report a better error message to the
user that he needs to fix the hostname.
At this point, my suggestion is to pick between the below two choices:
1. close the PR, given it's a corner case, and revisit in the future.
2. Go back to the original PR where the plan was to validate the URL when
Herder tries to forward the request to another node here
https://github.com/kpatelatwork/kafka/blob/7c8fb346a976903cc67e66c4ccfe6cc9858b5048/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java#L107
. This would require parsing the URL to create a URI and see if the host is
null then parse the host out of the URL. I tried
`System.out.println(new
java.net.URL("http://kafka_connect-0.dev-2:8080";).getHost());`
and it seems to do return the proper hostname.
What do you think if we go #2 approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530: URL: https://github.com/apache/kafka/pull/10530#issuecomment-824873571 Great catch @C0urante . Let me investigate what it takes to upgrade to an Apache HttpClient and how much of an effort it is. I Agree a seemingly simple change doesn't seem simple anymore so why not put the effort in the right future direction to use a client that supports IDNs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530: URL: https://github.com/apache/kafka/pull/10530#issuecomment-824141840 @tombentley I took another shot of implementing the PR using the above suggestions. Could you please check if it still needs improvement? I didn't do explicit IPV4 and IPV6 checks because the `uri.getHost()` won't be null if it's a valid address. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530: URL: https://github.com/apache/kafka/pull/10530#issuecomment-823359381 @tombentley Thanks for the comments, I resolved all of them :). Could you please review again and merge? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530: URL: https://github.com/apache/kafka/pull/10530#issuecomment-822663074 @tombentley could you please review and merge this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] kpatelatwork commented on pull request #10530: KAFKA-10231 fail bootstrap of Rest server if the host name in the adv…
kpatelatwork commented on pull request #10530: URL: https://github.com/apache/kafka/pull/10530#issuecomment-819667966 @kkonstantine and @rhauch Could one of you please review and merge this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
