KAFKA-4247: remove root from traversal path #1953
Conversation
…ry is not automatically included.
| @@ -40,6 +40,7 @@ | |||
| import org.apache.kafka.connect.tools.VerifiableSourceConnector; | |||
| import org.apache.kafka.connect.util.ConnectorTaskId; | |||
| import org.apache.kafka.connect.util.ReflectionsUtil; | |||
| import org.eclipse.jetty.util.log.Log; | |||
There was a problem hiding this comment.
Disallowed import (and doesn't look like it's used?)
There was a problem hiding this comment.
Not actually sure how that even got there to be honest. Will fix now
|
|
||
| builder.getUrls().remove(ReflectionsUtil.stringtoURL("/")); |
There was a problem hiding this comment.
Hmm, removing it here seems like it definitely solves the common case where this is a mistake. I can't really imagine anyone intending to pull in all of / unless they have some really tiny distribution, but it feels odd to special case it in the Java code when it's really an issue with how we construct classpaths in our scripts.
More generally, would this even work as expected for Windows?
There was a problem hiding this comment.
Yeah this was a silly idea, the issue should be handled at the time we construct the classpath as opposed to within the herder.
| @@ -42,6 +43,14 @@ public static void registerUrlTypes() { | |||
| Vfs.setDefaultURLTypes(urlTypes); | |||
| } | |||
|
|
|||
| public static URL stringtoURL(String path) { | |||
| try{ | |||
There was a problem hiding this comment.
This is a checkstyle failure (use ./gradlew testConnect to run all those tests and checkstyle without having to run the entire, very long test suite).
There was a problem hiding this comment.
This entire section is no longer needed since this will be handled in the script so I'll just remove it
| @@ -62,80 +62,80 @@ do | |||
| if [ -z "$CLASSPATH" ] ; then | |||
| CLASSPATH="$dir/*" | |||
| else | |||
| CLASSPATH="$CLASSPATH:$dir/*" | |||
| CLASSPATH="$dir/*":$CLASSPATH | |||
There was a problem hiding this comment.
We'd need to ensure the ordering works ok here as well. I'm not sure people will expect things to be prepended. If you've already set a CLASSPATH to override some jar, you won't get the same behavior anymore.
Also, do we know why have an empty classpath segment at the end doesn't cause the problem but at the beginning it works? And is it even a valid assumption across different JDKs?
There was a problem hiding this comment.
I'm not sure I understand why it should matter to a user if the $CLASSPATH they have set is appended rather than prepended to $dir. Either way they should end up with a $CLASSPATH variable which consists of both their input and the $dir variable.
I have only tried the Oracle JDK to be honest however this behavior is a result of the fact that the classloader defaults to the working directory in the absence of an properly set $CLASSPATH variable. Sine the abstract classloader scans these directories recursively this can potentially mean scanning the entire fs when the working directory is root.
I will try this out with OpenJDK as well however I expect to see similar results.
There was a problem hiding this comment.
It matters because if you want to override specific jars (e.g. with newer ones that are binary compatible but have bug fixes, such as compression libraries), you'd use classpath ordering to guarantee you load classes from the newer version rather than the older one.
There was a problem hiding this comment.
After doing some digging it looks like the reason appending CLASSPATH instead of prepending it works is due to the use of the String.split() method on the classpath environment variable in the Reflections API. An empty string would not be found at the end of the resulting array for a call to something like "test:classpath:".split(":") but would be at the beginning for something like ":test:classpath".split(":"). This functionality, being documented in the String API, seems like it'd be the same across every (valid) JDK implementation.
However, I think @ewencp's point about wanting to allow jar overriding still stands. A possible fix that keeps that option for the user would be to add the line CLASSPATH=${CLASSPATH#:} to the run-class script before before the JVM is started (and also before the necessary conversion to a Windows-compliant classpath in the event the user is running Cygwin) and after all the necessary directories/jars have been added to it. I'd be happy to create a separate PR for that fix if you don't have time, LMK!
|
New PR is up at #4406, as requested by @rnpridgeon offline. |
|
Closing in favor of #4406 |
Different fix for problem addressed by #1953. Should prevent the CLASSPATH environment variable from being prefixed by a single colon before the JVM is invoked in the run-class script, which will then prevent the current working directory from being unintentionally included in the classpath when using the Reflections library. If the current working directory should still be included in the classpath, it just needs to be explicitly specified either with its fully-qualified pathname or as a single dot ("."). Author: Chris Egerton <chrise@confluent.io> Reviewers: Randall Hauch <rhauch@gmail.com>, Jun Rao <junrao@gmail.com> Closes #4406 from C0urante/fix-colon-prefixed-classpath
No description provided.