hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wangda Tan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6840) Implement zookeeper based store for scheduler configuration updates
Date Tue, 12 Sep 2017 18:12:00 GMT

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

Wangda Tan commented on YARN-6840:
----------------------------------

Thanks [~jhung] for working on this patch, just reviewed overall workflows of the patch:

Thanks Jonathan, 

Apologize for the late review, some questions/comments:

In AdminService: 
- Move following code:
{code}
    if (isActiveTransition && isSchedulerMutable()) {
      try {
        ((MutableConfScheduler) rm.getRMContext().getScheduler())
            .refreshConfiguration();
      } catch (Exception e) {
        throw new IOException("Failed to refresh configuration:", e);
      }
    }
{code}
To refreshAll, and parameter {{isActiveTransition}} and {{private void refreshQueues}} can
be removed.
- Is it a better idea to move {{public void refreshQueues()}} logic to scheduler#reinitialize(...).
It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider.
(Please make sure that don't acquire any lock of scheduler while invoking ReservationSystem.reinitialize()).

In MutableCSConfigurationProvider: 
- It's better to remove:
{code}
  @VisibleForTesting
  YarnConfigurationStore getConfStore();
{code}
>From the base class. Unit test can call implementation's methods.

In MutableConfScheduler:
- Similar to above, it's better to remove:
{code}
  @VisibleForTesting
  MutableConfigurationProvider getMutableConfProvider();
{code}
>From the base class.
- refreshConfiguration -> reloadConfigurationFromStore

In ResourceManager: 
- {{createAndStartZKManager}} can be merged to rm#getZKManager(), maybe it's better to rename
it to rm#getAndInitializeZKManager(). Even if there's no race condition issue since this is
only called in RM initialization. It's better to add synchronize to the method.
- Getter API of ResourceManager should be exposed by RMContext. We should never use ref to
ResourceManager directly.

In YarnConfigurationStore:
- {{setResourceManager}} can be removed and you can pass RMContext ref to {{initialize}}.
- What happens if {{Configuration schedConf}} passed to a already initialized store?
- Could you add Javadocs to following methods:
{code}
  protected abstract Version getConfStoreVersion() throws Exception;

  protected abstract void storeVersion() throws Exception;

  protected abstract Version getCurrentVersion();
{code}

I will leave ZK-related implementation reviews  to [~xgong].

> Implement zookeeper based store for scheduler configuration updates
> -------------------------------------------------------------------
>
>                 Key: YARN-6840
>                 URL: https://issues.apache.org/jira/browse/YARN-6840
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Wangda Tan
>            Assignee: Jonathan Hung
>         Attachments: YARN-6840-YARN-5734.001.patch, YARN-6840-YARN-5734.002.patch, YARN-6840-YARN-5734.003.patch
>
>
> Right now there is only in-memory and leveldb based configuration store supported. Need
one which supports RM HA.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message