KAFKA-3615: Exclude test jars in kafka-run-class.sh

[Ishiihara]

@granders @hachikuji Can you take a look when you have time? Appreciate your time to review.

[ijuma]

This seems a bit too broad, can we not make the check a bit stricter?

[granthenke]

There are other files included too. Basically anything that gets built into */build/libs/* will show up be matched.

We should be sure to exclude javadoc, scaladoc, sources, test and test-sources jars.

[granthenke]

@Ishiihara Just want to confirm that this is only an issue when running from source right? When running from a release the only line that loads jars in kafka-run-class.sh is the following:

# classpath addition for release
CLASSPATH=$CLASSPATH:$base_dir/libs/*

It shouldn't stop this change but I would love to have kafka-run-class.sh work the same for both source builds and release builds. If not done here I can open a jira to track that.

[Ishiihara]

@ijuma @granthenke Thanks for the review. I updated the patch to have more specific filtering condition and also make sure that we also filter files in case that we run from binary package.

[ewencp]

@Ishiihara In build.gradle we have a task called systemTestLibs that depends on some test jars. I think that might be because we have some tests that execute code that's in the test jars. I think @granders set that up so maybe he can clarify how that's used. My concern is that filtering out all the test jars might break some of those tests. If we need support for running some classes in test jars then we might need to make it possible to toggle this filtering on/off (though filtering out by default seems like a good choice).

[granders]

@Ishiihara @ewencp
Yeah I have the same concern - this may break a few tests which depend on classes in the test jar - e.g. the stream smoke tests.

[ijuma]

systemTestLibs was originally added because we need minikdc for the system tests. And then streams also started using this (as @granders said). It seems to me that we should have a variable that needs to be set for the test jars to be included. It is unexpected to have that as the default behaviour in my opinion.

[granders]

An environmental variable to trigger addition of test jars seems reasonable

[ewencp]

@ijuma @granders Agreed on the default behavior.

[Ishiihara]

@ewencp @ijuma @granders Thanks for the review, I would add an environment variable to control test jar addition.

[Ishiihara]

Thanks for the review. Added a INCLUDE_TEST_JARS variable and change filter_file to should_include_file according to @granders suggestion.

[ewencp]

@Ishiihara but presumably this flag needs to be enabled in some of the system tests?

[granders]

@Ishiihara Offhand, I think Minikdc service and streams-related services will need this flag (in system test code)

And then it would probably be good to run in a branch builder

[Ishiihara]

@granders Added include test jars to relevant system test services.

[ewencp]

Leftover debug output

[ewencp]

This LGTM, I'll just remove that one extra echo during commit

[Ishiihara]

@ewencp Thanks for the review, removed the left over debug output.

[dpkp]

this is broken. It should be for file in $base_dir/libs/*; ... without the glob, you're looping over 1 entry: "libs".

I believe this bug breaks the classpath for all scripts in bin/ in the release artifact