[
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: [email protected]
For additional commands, e-mail: [email protected]
