1. TO DO : Comment for getQueueConfigurationParser is not required.
Dont think there is any need of introducing parser.parse method. As for , there will always be 1 parser that is QueueConfigurationParser
and existing DeprecatedConfigurationParser is just for deprecation and would go away.
2.The parameter name in "synchronized void refreshQueues(Configuration conf, QueueRefresher scheduler)"
) should be refresher instead of scheduler.
1.QueueRefresher should be an abstract class with refreshQueues being abstract.
Can it be something like this:
abstract class QueueRefresher
abstract void refreshQueues(List<JobQueueInfo> newRootQueues)
1.In QueueState no need for enumMap. QueueState.valueOf(String name) would give the QueueState object.
2.newState.getChildren().size() state in isHierarchySame would return NullPointerException
incase of no children.
3.Do we need to define an extra state of UNDEFINED in QueueState , as Queues with no state would have a default state of RUNNING.
which is same behaviour before states were introduced.
It basically boils down to the question , what is the value of state in case of queues with children.
1. Remove instance of schedConf.
CapacitySchedulerConf constructor should just be instantiated and get the value most of the time we any how use QueueSchedulingContext values.
Incase of not passing any properties ,CapacitySchedulerConf would give default values.
Incase of distributedUnConfiguredCapacity some of the testcases directly check the configuration xml.So they need to be taken care of.
2. Remove SchedulingDisplayInfo object alltogether , instead pass QueueSchedulingContext directly.
Also check incase above case do we need queueNameToQueueContextMap anymore.
3. We can remove the synchronized block from initializeQueues , instead we can have more fine grained locking
when we just change root or update root or not at all.
4. We can move most of the code inside initializeQueues to QueueHierarchyBuilder.
so initializeQueues becomes following :
QueueHierarchyBuilder builder = new QueueHierarchyBuilder();
//do current stuff
this.root = builder.getRoot();
5. instead of updateConfigurationOfAbstractQueues in CapacityTaskScheduler ,
can we move this method to AbstractQueues class ,
so we can call root.refresh(newRoot) such that parents let there children update themselves first then parents update themselves.
so refreshing logic becomes simply as.
6.updateQueueMaps updates jobQueueManager unnecessarily as the list of JobQueue should not be changed in case of refresh.
this method might need to be revisited once we implement 2. This would also remove the need of synchronization in JobQueuesManager
7.getOrderedQueue signature should be reverted back to return AbstractQueue
8.initializationPoller instance variable scope is changed to package it shuold be private
9.getDisplayInfo neednot be synchronized as it is only reading data.
1. It is incorrect to change the signature of getDescendentJobQueues to return List of JobQueue. return List of AbstractQueue provides
the correct abstraction. JobQueue is just another specialized AbstractQueue with containers for JIP.
1. We dont need to synchronize any of the methods in JobQueuesManager. As it is always called from JobTracker in synchronized block
also the queue structure jobQueues is only created once and never changed. double check ??
1.Removal of unused code. not related to patch though
1. there is no need for maintaining the fullyQualifiedName and name separatly . Instead we shuold always use fullyQualified name everywhere
and can provide method to get the normal queueName.