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 [email protected] or file a JIRA ticket
with INFRA.
---
