[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Github user asfgit closed the pull request at: https://github.com/apache/metron/pull/600 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120434003
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java
---
@@ -68,12 +68,14 @@
return ret;
}
- public List fromEndpoint(String url) throws URISyntaxException {
+ public List fromEndpoint(String url){
List ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+String hostPort = Iterables.getLast(splits);
+ret.add(hostPort);
--- End diff --
@mattf-horton You're absolutely not being a PITA. Honestly, I tried to get
a solution to a known problem out a little too quickly because it was actually
experienced, so I'm actually pretty happy you're double checking me on this so
something half baked didn't go in.
So looking back at this a bit more fresh, this only gets called from
`getBrokersFromZookeeper`. The fact that's it's public isn't reflective of
it's use (I'd probably argue package private, so it can be unit tested, or just
better unit tested in general).
In practice, these aren't general URLs. They're specifically from
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+data+structures+in+Zookeeper
e.g.
```
  "endpoints": ["PLAINTEXT://host1:9092", "SSL://host1:9093"]
```
So it's exactly protocol://host[:port] (to the best of my knowledge). So
while the URI solution is probably technically better (outside of the
underscore issue), fixing the split should be perfectly fine, if a bit kludgier.
Looking back at it, I'm pretty sure I didn't hit the right code path to
trigger it in testing. I'm going to adjust the unit tests to actually do
something more useful, so we avoid these issues in the future. I'm also going
to add a comment, so it's very explicit what the actual assumptions made by
that function are.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Github user mattf-horton commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120429871
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java
---
@@ -68,12 +68,14 @@
return ret;
}
- public List fromEndpoint(String url) throws URISyntaxException {
+ public List fromEndpoint(String url){
List ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+String hostPort = Iterables.getLast(splits);
+ret.add(hostPort);
--- End diff --
Thanks, @justinleet , and sorry if I'm being a PITA. By inspection, I have
the following concerns:
1. The URI solution always delivers exactly "host[:port]" (or fails). The
split solution delivers the entire url except the protocol, ie
"host[:port]/path/extension/if/any...", unless there's something upstream
cutting off the path extension (if any). Is there? I didn't see anything, it
looks like endpoint urls are simply read raw from a config JSON structure. So
I would have expected trimming the proposed split at both ends. Unless
endpoint URLs are supposed to allow path extensions, in which case the URI
approach is clearly wrong.
2. The split code only returns a result if the split size is == 2. This
prevents it from accepting:
* Endpoint urls without the protocol prefix
* Endpoint urls with path extensions that accidentally have double
slashes in them (a common problem with machine-generated URLs, and accepted by
most URL path processors)
It also does not provide any log feedback on why it silently ignores
"bad" endpoint urls.
None of these issues would show up in testing unless the unit tests include
thorough negative testcases for invalid configured endpoint URLs.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/600#discussion_r120369355
--- Diff:
metron-platform/metron-common/src/main/java/org/apache/metron/common/utils/KafkaUtils.java
---
@@ -68,12 +68,14 @@
return ret;
}
- public List fromEndpoint(String url) throws URISyntaxException {
+ public List fromEndpoint(String url){
List ret = new ArrayList<>();
if(url != null) {
- URI uri = new URI(url);
- int port = uri.getPort();
- ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
+ Iterable splits = Splitter.on("//").split(url);
+ if(Iterables.size(splits) == 2) {
+String hostPort = Iterables.getLast(splits);
+ret.add(hostPort);
--- End diff --
The problem is that the URI based solution that was implemented in that
patch (I believe based on my suggestion, sigh) doesn't allow for protocols with
underscore characters. It's entirely independent of SASL_PLAINTEXT, other than
the fact that underscore is in it.
The intention was to exactly reverse the chunk you mention, because the
original didn't care about underscores.
On further inspection (which I'm glad you did!), these are not equivalent
before and after. Which begs the question of why things worked in testing.
I'll dig in a bit more to figure out what's going on under the hood here.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
GitHub user justinleet opened a pull request: https://github.com/apache/metron/pull/600 METRON-976 KafkaUtils doesn't handle SASL_PLAINTEXT ## Contributor Comments Used @cestella's original implementation before I apparently foolishly assumed we'd get Java compliant URIs. See: https://github.com/apache/metron/pull/486/commits/5d36c79effec75f6ac95fa587f80da0bd5420135 Unit testing is updated, and now checks that SASL_PLAINTEXT works (not just PLAINTEXTSASL). I still need to spin up full dev. @cestella Let me know how you'd like to handle credit, given that you originally wrote it. ## Pull Request Checklist Thank you for submitting a contribution to Apache Metron. Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions. Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides. In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel). - [x] Does your PR title start with METRON- where is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. - [x] Has your PR been rebased against the latest commit within the target branch (typically master)? ### For code changes: - [] Have you included steps to reproduce the behavior or problem that is being changed or addressed? - [ ] Have you included steps or a guide to how the change may be verified and tested manually? - [ ] Have you ensured that the full suite of tests and checks have been executed in the root incubating-metron folder via: ``` mvn -q clean integration-test install && build_utils/verify_licenses.sh ``` - [x] Have you written or updated unit tests and or integration tests to verify your changes? - [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent? Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request. You can merge this pull request into a Git repository by running: $ git pull https://github.com/justinleet/metron METRON-976 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/metron/pull/600.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 #600 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
