metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mattf-horton <...@git.apache.org>
Subject [GitHub] metron pull request #600: METRON-976 KafkaUtils doesn't handle SASL_PLAINTEX...
Date Tue, 06 Jun 2017 17:43:45 GMT
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<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 --
    
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message