The two plots show the time spent in millisecconds in cluster initialization for old and new implementation. There is a considerable drop after the change. The test was a simple unit test to record the the time spent doing the cluster initialization per thread. There are a total of 5000 such threads calling cluster initialization() .

Jason Lowe Request you to please share comments. Thank you!

-1 overall

This message was automatically generated.

Corrected checkstyle issues.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Thanks for the patch, Kuhu!

There's a race condition in the initialization of the provider list. A thread can try to init while another thread is busy building the provider list. In that case the second thread may try to walk the ArrayList while it is being constructed since providerList.isEmpty() will be false yet the list isn't fully constructed. This could cause a client to fail to find a provider that should have been found and violates the concurrency safety of ArrayList since we're reading it while it's being written. We could use a volatile that's initialized to null when it hasn't been set. That way either the list isn't there and we'll synchronize the threads that are trying to create it within the null check block or the list will be there in its entirety and not modified beyond that.

Nit: initProviderList is probably a better name since set implies the ability to provide the value to set it to

Added a volatile boolean to ensure the provider list is read only after it has been fully populated. Removed the list.isEmpty() check.

Curious, why are we using a separate boolean rather than just using volatile List<...> providerList = null and checking if providerList is null to see if it needs to be initialized? The separate boolean seems unnecessary.

Thank you Jason Lowe .
I thought of making the list volatile. Here is a scenario which I felt could be a problem.

Thread T1 comes into initialize() , calls initProviderList(), checks that the list is null, which is true the first(few) times. It gets into the sync block, since no one is in there. T1 hasn't finished populating the list but has initialized it (so now the list == null is false) and right then another Thread T2, calls initProviderList() checks that the list is still null, gets false and returns to initialize() , now it starts iterating over the list which is still being written by T1. This case would happen right at the beginning but since I suspected it being ideal for a concurrency exception, I went with another volatile object. If we remove the first (list == null) check before the sync block and have only the one inside it, then every thread has to contend for the sync block even if it is to check whether the list is initialized or not. If the latter case is not a concern then making the list volatile works.

The problem is this step:

T1 hasn't finished populating the list but has initialized it

and the fix is to use a local variable while the list is being initialized then set providerList to the completely built list when we're done.

Got it! Thank you Jason Lowe. Updated patch.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Jason Lowe, Eric Payne Request for review comments. Thanks a lot.

-1 overall

This message was automatically generated.

Kuhu Shukla, Thanks for documenting this problem and providing the patch for it.

It looks like the following could happen in Cluster.java:

• thread 1 enters initProviderList and begins to process the entries in frameworkLoader
• thread 2 enters initProviderList, sees that providerList is null, and exits. Then, in initialize, no processing for providerList would happen and it would throw an IOE when clientProtocolProvider is null. Am I missing something?

Thanks Eric Payne for your comments.

In the above scenario when thread 2 comes along and finds that the provider list is null , since thread 1 is already in the synchronized block, thread 2 will wait and enter it only when thread 1 is done. Once it does get into the block, an additional check of whether the list is null or not helps thread 2 to not do the same work again. Please let me know if this helps or if I am missing anything? Thanks a lot again.

Thanks for updating the patch, Kuhu!

Why are we making a copy of the localProviderList when initializing providerList instead of just doing providerList = localProviderList? I understand why we don't want to use providerList directly when initializing the array since we don't want to expose a partially initialized array to another thread, but I don't see what the copy accomplishes when the local array list is built.

Please address the checkstyle nits about the brace placement since it deviates from the coding standard. I realize it was that way before, but since we're touching the code we might as well fix it.

Thank you Jason Lowe for your comments. I corrected the checkstyle and the array copy issue.

-1 overall

This message was automatically generated.

In the above scenario when thread 2 comes along and finds that the provider list is null , since thread 1 is already in the synchronized block, thread 2 will wait

Kuhu Shukla, Yes, you are correct. I had missed that. Thanks for the clarification.

+1 (non-binding)

+1 committing this.

Thanks to Kuhu for the contribution and to Eric for additional review! I committed this to trunk, branch-2, and branch-2.8.

FAILURE: Integrated in Hadoop-trunk-Commit #9098 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9098/)
MAPREDUCE-6473. Job submission can take a long time during Cluster (jlowe: rev f657b54281d517c69df0a23b97b47befa77d1dc2)

• hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/Cluster.java
• hadoop-mapreduce-project/CHANGES.txt