KAFKA-13971: Fix atomicity violations caused by improper usage of ConcurrentHashMap - part2

[divijvaidya]

## Problem #1 in DelegatingClassLoader.java
Atomicity violation in example such as:
Consider thread T1 reaches line 228, but before executing context switches to thread T2 which also reaches line 228. Again context switches to T1 which reaches line 232 and adds a value to the map. T2 will execute line 228 and creates a new map which overwrites the value written by T1, hence change done by T1 would be lost. This code change ensures that two threads cannot initiate the TreeMap, instead only one of them will.

Problem #2 in RocksDBMetricsRecordingTrigger.java

Atomicity violation in example such as:
Consider thread T1 reaches line 40 but before executing it context switches to thread T2 which also reaches line 40. In a serialized execution order, thread T2 should have thrown the exception but it won't in this case. The code change fixes that.

Note that some other problems associated with use of concurrent hashmap has been fixed in #12277

[ijuma]

Is the relevant code specified as thread safe?

[divijvaidya]

Is the relevant code specified as thread safe?

Thank you for your review @ijuma. I appreciate it. Though, I am afraid I don't understand your question.

Are you asking whether the existing code is supposed to be thread safe?
If yes, for DelegatingClassLoader.java the javadoc for the class mentioned that it is supposed to be thread safe (but it isn't due to the bug that is fixed in this review). For the RocksDBMetricsRecordingTrigger.java, we run a thread periodically from a metric trigger thread pool which reads from the map maintained in the class. At the same time it is possible that another thread is mutating the map during startup/shutdown of rocksDB which may leave the map in inconsistent state. Hence, it's important for this class to be thread safe as well.
Also, note that both the classes in this review use ConcurrentHashMap (albeit incorrectly) to ensure thread safe mutation over the map.

Are you asking whether the changed code is thread safe?
If yes, the change uses atomic operations provided by ConcurrentHashMap to ensure thread safety.

[divijvaidya]

@C0urante please review when you get a chance.

[C0urante]

This is a nice improvement in readability, but are we certain it's necessary? I commented on the change to the plugin scanning logic in Connect because I'm familiar with that part of the code base; I don't have the same familiarity with Streams, though.

I think it's fine to merge this change, but if this method isn't intended to be invoked concurrently, we should modify the PR title so that the commit message doesn't imply this is a bug fix and instead recognizes it as a cosmetic improvement.

[mdedetrich]

I agree with @C0urante here, if the method isn't meant to be invoked in a concurrent situation then we should either revert to the original containsKey OR add a comment along the lines of "Currently this code isn't called in a concurrent situation"

[divijvaidya]

Thanks for your code review folks. On deeper analysis, it doesn't look like that this code will be invoked by multiple threads. I will discard this PR now and I thank you again for looking into this PR.