[
https://issues.apache.org/jira/browse/YARN-10552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17277991#comment-17277991
]
Siddharth Ahuja commented on YARN-10552:
----------------------------------------
Hey [~snemeth], thanks a lot for the de-duplication here!
Few comments from my side:
# SLSSchedulerCommons - Can we please explicitly assign a default value for the
declared fields like metricsOn etc. and not rely on Java to assign one, just as
a good programming style.
# Class variables - metricsOn & schedulerMetrics could be marked as private in
SLSSchedulerCommons, new getters should be defined that could be invoked within
the individual scheduler classes instead of referring them directly from a
separate object.
# The "Tracker" seems to be common to both schedulers as such we could move the
declaration & initialization to the common SLSSchedulerCommons, implement
getTracker() here that returns the tracker object and keep getTracker() in the
individual schedulers (we have to, thanks to SchedulerWrapper) and just return
the tracker by calling schedulerCommons.getTracker().
# //metrics off, //metrics on comments inside handle() in SLSSchedulerCommons
don't seem to be adding much value so let's just remove them.
# appQueueMap was not present in SLSFairScheduler before (it was in
SLSCapacityScheduler) however from
https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-sls/src/main/java/org/apache/hadoop/yarn/sls/scheduler/SLSFairScheduler.java#L163,
it seems that the super class of the schedulers -
https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java#L159
has this already. As such, do we really need to define a new map as a common
map at all in SLSSchedulerCommons or can we somehow reuse the super class's
map? It might need some code updates though.
# In regards to the above point, considering SLSFairScheduler did not
previously have any of the following code in handle() method:
{code}
AppAttemptRemovedSchedulerEvent appRemoveEvent =
(AppAttemptRemovedSchedulerEvent) schedulerEvent;
appQueueMap.remove(appRemoveEvent.getApplicationAttemptID());
} else if (schedulerEvent.getType() ==
SchedulerEventType.APP_ATTEMPT_ADDED
&& schedulerEvent instanceof AppAttemptAddedSchedulerEvent) {
AppAttemptAddedSchedulerEvent appAddEvent =
(AppAttemptAddedSchedulerEvent) schedulerEvent;
SchedulerApplication app =
(SchedulerApplication)
scheduler.getSchedulerApplications().get(appAddEvent.getApplicationAttemptId()
.getApplicationId());
appQueueMap.put(appAddEvent.getApplicationAttemptId(), app.getQueue()
.getQueueName());
{code}
Do you think this was a bug that wasn't earlier identified?
> Eliminate code duplication in SLSCapacityScheduler and SLSFairScheduler
> -----------------------------------------------------------------------
>
> Key: YARN-10552
> URL: https://issues.apache.org/jira/browse/YARN-10552
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Minor
> Attachments: YARN-10552.001.patch
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
