Thanks Arun Suresh for the review, actually I'm working on the patch
AbstractYarnScheduler::getApplicationAttempt() : Do you need the read lock ...
AbstractYarnScheduler::moveAllApps() : Don't think we need a writelock...
The reason I put a writelock because we want the move operation to be serialized, for example, it may have some issue when we do two moveAll operation at the same time. To me moveApp is equivalent to remove the app and add the new one. Same as we protect addAppAttempt, we should use the same writelock for safety.
SchedulerApplicationAttempt::pendingRelease :Maybe use a ConcurrentSkipListSet ?
Unless we need to order the set, I think use ConcurrentHashSet should be enough, SkiplistSet gets worse performance (O(logN) vs. O(1))
Considering that initScheduler is called exactly once, and the Scheduler cannot be used before it is initted, do we need a write lock there ?
I would prefer keep it just for safety and consistency, as it doesn't affect performance. Same for stop and startSchedulerThreads
FairScheduler 2.. 3..
And Jian He,
I revisited checkSchedContainerChangeRequest, it is actually safe, so I removed comment of the original method.