[
https://issues.apache.org/jira/browse/YARN-10623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17291728#comment-17291728
]
Peter Bacsko commented on YARN-10623:
-------------------------------------
Thanks [~zhuqi] for the update. I missed a couple of things in my previous
review.
1.
{noformat}
clock = SystemClock.getInstance();
{noformat}
I suggest using {{MonotonicClock}}. Looking at the comments of {{SystemClock}},
that class is a better choice.
2. {{waitFor}} usage can be simplified:
{noformat}
GenericTestUtils.waitFor(() ->
cs.getConfiguration().getMaximumSystemApplications() != 10000,
500L, 10_000L);
{noformat}
I think 100ms interval time is a bit small, 500ms seems to be a better
compromise. Watch out for the line length or checkstyle will complain.
3.
{noformat}
configuration.set(YarnConfiguration.RM_CONFIGURATION_PROVIDER_CLASS,
"org.apache.hadoop.yarn.FileSystemBasedConfigurationProvider");
configuration.set(YarnConfiguration.RM_CONFIGURATION_PROVIDER_CLASS,
"org.apache.hadoop.yarn.FileSystemBasedConfigurationProvider");
{noformat}
You can also use
{{FileSystemBasedConfigurationProvider.class.getCanonicalName()}} here.
4.
{noformat}
csConf.set(CapacitySchedulerConfiguration.MAXIMUM_SYSTEM_APPLICATIONS,
"5000");
{noformat}
For consistency reasons, I suggest using {{setInt()}} here.
5.
{noformat}
@VisibleForTesting
public long getLastReloadAttempt() {
return lastReloadAttempt;
}
@VisibleForTesting
public long getLastModified() {
return lastModified;
}
@VisibleForTesting
public Clock getClock() {
return clock;
}
@VisibleForTesting
public boolean getLastReloadAttemptFailed() {
return lastReloadAttemptFailed;
}
{noformat}
As I can see, these methods are only called from test cases. You can reduce the
visibility to package private, so just remove "public".
6. Note that stack traces are still not visible in the latest patch:
{noformat}
LOG.error("Can't refresh queue: " + e.getMessage());
...
LOG.error("Can't get file status for refresh : " + e.getMessage());
{noformat}
This is preferred:
{noformat}
LOG.error("Can't refresh queue", e);
...
LOG.error("Can't get file status for refresh", e);
{noformat}
> Capacity scheduler should support refresh queue automatically by a thread
> policy.
> ---------------------------------------------------------------------------------
>
> Key: YARN-10623
> URL: https://issues.apache.org/jira/browse/YARN-10623
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: capacity scheduler
> Reporter: Qi Zhu
> Assignee: Qi Zhu
> Priority: Major
> Attachments: YARN-10623.001.patch, YARN-10623.002.patch,
> YARN-10623.003.patch, YARN-10623.004.patch, YARN-10623.005.patch
>
>
> In fair scheduler, it is supported that refresh queue related conf
> automatically by a thread to reload, but in capacity scheduler we only
> support to refresh queue related changes by refreshQueues, it is needed for
> our cluster to realize queue manage.
> cc [~wangda] [~ztang] [~pbacsko] [~snemeth] [~gandras] [~bteke] [~shuzirra]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
