[ 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