KAFKA-10579: Upgrade reflections from 0.9.12 to 0.10.2

[gharris1727]

The prior version of Reflections had two bugs:

1. Errors were propagated differently with parallel execution, which was already locally patched with InternalReflections.
2. An internal data structure had a data race, that requires synchronization.

Both of the above were patched upstream and are included in 0.10.2. This is an alternative to #14020 where we patch it ourselves and keep the old version. Some of the syntax has changed, so adjust the call-sites to accommodate this.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[divijvaidya]

Please provide a link to the complete release notes between these two versions so that we can verify that there are no other diffs. For example, in the past on analyzing the diff we have found bugs such as newer versions only work with JDK 11+ etc. Hence, analysis of the release notes is useful.

[divijvaidya]

Also, please add your recommendation on the backporting decision for this PR.

[gharris1727]

Release notes:

reflections-0.10 refactor
known issue ronmamo/reflections#337: annotation not marked with Retention(RUNTIME) will be excluded because of an exception

reflections-0.10.1
0.10.1 - fix exception in JavassistHelper.getParametersAnnotations, docs
ci: Setup GitHub actions for basic PR CI (ronmamo/reflections#333)
Make the path format to be platform-compatible (ronmamo/reflections#299)
enable Dependabot v2 (ronmamo/reflections#319)
Enable automatic module name (ronmamo/reflections#308)
known issue ronmamo/reflections#351: deprecated scanners are using wrong index name :( which results in empty query results. will be solved in next release. workaround/solution: migrate to the new Scanners

reflections-0.10.2

https://github.com/ronmamo/reflections/releases

ronmamo/reflections@0.9.12...0.10.2

[gharris1727]

The license for this changed:

- The license is [WTFPL](http://www.wtfpl.net/), just do what the fuck you want to. 
+ Dual licenced with Apache 2 and [WTFPL](http://www.wtfpl.net/), just do what the fuck you want to.  

I've added it under both areas in LICENSE-binary, I'm assuming that both licenses apply to the new version but I don't know for sure.

[gharris1727]

The problem that this fixes is relatively rare, so backporting does not need to be a priority. It should be safe to however, as the scanning behavior should be unchanged.

[divijvaidya]

Looks good. Separately, I believe we should migrate away from this library since it not under active maintenance since 2021. This poses a supply chain risk to this project. I have created a JIRA to consider removing this dependency - https://issues.apache.org/jira/browse/KAFKA-15203

[C0urante]

@gharris1727 since the issue being addressed here occurs fairly infrequently, have you done any testing (local or CI) to try to gauge with reasonable confidence that the dependency bump actually fixes this bug? FWICT the release notes don't call out concurrency issues or patches for them.

[gharris1727]

since the issue being addressed here occurs fairly infrequently, have you done any testing (local or CI) to try to gauge with reasonable confidence that the dependency bump actually fixes this bug? FWICT the release notes don't call out concurrency issues or patches for them.

No, I haven't done testing to verify the bug is fixed, but I manually inspected the code. I found that they made a similar change to my patch, making the analogous innermost collection synchronized: ronmamo/reflections@0.9.12...0.10.2#diff-14d1b9286b6f615ad4dee1ed63d2faf97e063b6c167426762a7145e350b7d345R170

Map<String, Set<Map.Entry<String, String>>> collect = configuration.getScanners().stream().map(Scanner::index).distinct()
.collect(Collectors.toMap(s -> s, s -> Collections.synchronizedSet(new HashSet<>())));

The type is no longer a List but a Set<Map.Entry<String, String>>, and the concurrent operation is Set#addAll instead of List#add, but it is used in the same capacity to collect results from the parallel threads.

[C0urante]

Thanks for the link! The GitHub UI is a little wonky with large diffs so it was a little hard to find the section you pointed to.

I believe this is the part you're referencing: https://github.com/ronmamo/reflections/blob/0.10.2/src/main/java/org/reflections/Reflections.java#L167-L170

public class Reflections implements NameHelper {
// ...
    protected Map<String, Map<String, Set<String>>> scan() {
        long start = System.currentTimeMillis();
        Map<String, Set<Map.Entry<String, String>>> collect = configuration.getScanners().stream().map(Scanner::index).distinct()
            .collect(Collectors.toMap(s -> s, s -> Collections.synchronizedSet(new HashSet<>())));
    // ...
    }
// ...
}

It also looks like the fix in #14020 was implemented upstream in ronmamo/reflections#275 (see the change to the put method in Store.java) but never released, since the next PR (ronmamo/reflections#335) was a large refactor that moved that logic back into the Reflections class and included the change you linked to.

Given this, I'm convinced that the dependency bump at least contains an attempt at a fix for the concurrency bug we've been seeing. I'd love it if we could get a better guard against regression, though. Would it be too much to ask to run system tests? I know they're a bit much for a simple dependency bump, but considering how infrequently the bug occurs and how vital this library is to Connect right now (workers can't start up without it), it seems like we don't have a ton to gain and a fair amount to lose.

[gharris1727]

Would it be too much to ask to run system tests?

I ran the connect system tests on a spare machine and got 181/181 passing.

[C0urante]

Thanks for running the system tests! LGTM

I also couldn't resist the temptation to try to reproduce the concurrency bug locally. After some experimentation I achieved about a 60% failure rate by using OpenJDK 8 (doesn't work with later versions due to a change in the implementation for ArrayList) to run a unit test that used the ReflectionScanner to scan a single directory that contained 10,000 copies of a JAR file which itself only contained a manifest and the FileStreamSourceConnector class.

With this patch applied, I was able to run the exact same test case 100 times consecutively without a failure, or any other plugin scanning errors.

At this point I think we've done our due diligence to ensure that this bump is safe. Feel free to merge 👍

[gharris1727]

Test failures appear unrelated, and this has passed unit tests, system tests, and stress testing already.