[
https://issues.apache.org/jira/browse/HADOOP-4513?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12648951#action_12648951
]
Hemanth Yamijala commented on HADOOP-4513:
------------------------------------------
Some comments:
- Please expose variables like initalizationPoller, that are needed in tests
via accessor methods, and make the variable itself private.
JobInitializationPoller:
- Typo: Should be {{JobInitializationPoller}} and not
{{JobInitalizationPoller}}. Also, same typo (initaliz..) exists for other
variables also, including in capacity scheduler.
- Need not call super() explicitly.
- maxUserToInitialize should be maxUsersToInitialize
- Please document the purpose of the various data structures.
- {{terminate}} should use the logger and not print stack trace while handling
InterruptedException. And it should probably break here, and exit.
- {{terminate}} is interrupting and joining threads more than once. While this
may not be a problem, it is best not to do so. The code should check if the
thread is still alive before calling these APIs.
- {{threadsAssignedToQueue}} seems to indicate more than one thread can be
assigned to the same queue. Suggestions for renaming include
{{threadsAssignedToQueues}} or {{threadsToQueuesMap}}. Similarly,
{{assignThreadsToQueue}}.
- Initialization of threadsAssignedToQueue can be in the constructor itself.
- Since threadsAssignedToQueue is a member variable, we don't need to pass it
to assignThreadsToQueue
- commented line "// startIndex = ((startIndex+1)%countOfQueues);" can be
removed.
- Some comments on simplifying {{getJobsToInitialize}}
-- need more comments explaining the code. Particularly why the code is
checking for total number of jobs per queue is not obvious.
-- maxJobsAllowedToInitalize should be maxJobsPerUserAllowedToInitialize
-- would the (pseudo-code) below do the same thing ? It seems like this is a
bit more clear.
{code}
if (!isUserPresent
&& userJobsInitalized.size() <
maximumUsersAllowedToInitialize) {
// new user and we are within limits, so initialize
userJobsInitalized.put(user, Integer.valueOf(1));
jobsToInitalize.add(job);
initalizedJobs.add(job.getJobID());
countOfJobsInitialized++;
} else if (isUserPresent
&& numberOfJobs < maxJobsAllowedToInitalize
&& countOfJobsInitialized < maxJobsPerQueueToInitialize) {
// existing user, we are within overall limits and per user limit
userJobsInitalized.put(user, Integer.valueOf(1));
jobsToInitalize.add(job);
initalizedJobs.add(job.getJobID());
countOfJobsInitialized++;
}
{code}
- printJobs should be called only if the Log is enabled (LOG.isDebugEnabled)
- When initTasks fails, we are calling job.fail. Maybe we should also notify
the jobqueuemanager to remove the job from the queue ? This needs some more
discussion.
- In the worker thread's run, the {{continue}} seems redundant, as there's no
other code to execute.
- The comment on MAX_ADDITIONAL_USERS_TO_INIT can be improved a bit. Basically,
we want to keep a couple of additional users jobs initialized so when their
jobs need to run because of user limits, they would be ready.
- Do we need a ConcurrentHashMap for the queue to threads mapping ? The
modification is done in the addQueue method which is only done once, and all
subsequent operations are read only.
- The {{terminate}} method should be package private.
- The worker thread's shutdown should be called before initializing
- We should check if the stop flag (or equivalent) is set before going into
Thread.sleep. Alternative is to use Thread.isInterrupted().
JobQueueManager:
- I am thinking removal of completed jobs from the 'jobqueue' must also be done
in jobCompleted, and not from the poller. This keeps it simple to understand.
CapacityTaskScheduler:
- Use {{terminate}} to stop the Poller thread, rather than {{stop}}
CapacitySchedulerConf:
- {{maximum-initialized-jobs-per-user}} should be included in the xml file. I
am thinking we should define the values for the default queue atleast. It is
preferable to define the default value for all queues as well - just for other
variables. This will keep the variables consistent.
- Check for invalid (negative values) is not done for
maximum-initialized-jobs-per-user.
> Capacity scheduler should initialize tasks asynchronously
> ---------------------------------------------------------
>
> Key: HADOOP-4513
> URL: https://issues.apache.org/jira/browse/HADOOP-4513
> Project: Hadoop Core
> Issue Type: Bug
> Components: contrib/capacity-sched
> Affects Versions: 0.19.0
> Reporter: Hemanth Yamijala
> Assignee: Sreekanth Ramakrishnan
> Fix For: 0.19.1
>
> Attachments: HADOOP-4513-1.patch, HADOOP-4513-2.patch
>
>
> Currently, the capacity scheduler initializes tasks on demand, as opposed to
> the eager initialization technique used by the default scheduler. This is
> done in order to save JT memory footprint. However, the initialization is
> done in the {{assignTasks}} API which is not a good idea as task
> initialization could be a time consuming operation. This JIRA is to move out
> the initialization outside the {{assignTasks}} API and do it asynchronously.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
