[ 
https://issues.apache.org/jira/browse/YARN-10623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17290013#comment-17290013
 ] 

Gergely Pollak commented on YARN-10623:
---------------------------------------

[~zhuqi] thank you for the patch!

I really like the idea that the solution is a configurable extra component 
(scheduling edit policy), this way it is cleanly decoupled from the main code 
base, won't cause any issue for those who don't want to use it. 

I very much agree with [~gandras] that last modification check was necessary 
however I think it introduced a bug:

 
{code:java}
126        if (lastModified > lastSuccessfulReload &&
127            time > lastModified + monitoringInterval) {
{code}
So in the edge case when you update this file MORE frequently than the 
monitoringInterval (which is configurable, so it might be in the minute or even 
10 minutes range), then you won't ever refresh the config, since last modified 
+ monitoringInterval will be ALWAYS greater than the current time. I think you 
should go with

 
{code:java}
126 if (lastModified > lastSuccessfulReload &&
127   time > lastSuccessfulReload + monitoringInterval) {
{code}
Or even better introduce a lastReloadAttempt, since a reload can fail, and in 
this case it would result keep trying to reload the invalid configuration, so 
if you'd introduce a lastReloadAttempt and set it each time before you try to 
reload the configuration, then you could use 
{code:java}
126 if (lastModified > lastReloadAttempt && 
127   time > lastReloadAttempt + monitoringInterval) {
{code}
This would guarantee that you don't reload more frequently than the 
monitoringInterval, you don't reload if the configuration hasn't been modified, 
but still keep reloading if the file is updated frequently.

Otherwise the patch looks good to me (non-binding).

> 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
>
>
> 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: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to