metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From justinleet <...@git.apache.org>
Subject [GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Date Tue, 06 Jun 2017 17:59:06 GMT
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<String> fromEndpoint(String url) throws URISyntaxException {
    +  public List<String> fromEndpoint(String url){
         List<String> ret = new ArrayList<>();
         if(url != null) {
    -      URI uri = new URI(url);
    -      int port = uri.getPort();
    -      ret.add(uri.getHost() + ((port > 0)?(":" + port):""));
    +      Iterable<String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message