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

Bikas Saha commented on YARN-1068:
----------------------------------

Should this be RMHAServiceProtocol address? Admins and ZKFC would be connecting 
on this protocol right?
{code}
+  public static final String RM_HA_ADMIN_ADDRESS =
+      RM_HA_PREFIX + "admin.address";
...
+    <description>The address of the RM HA admin interface.</description>
+    <name>yarn.resourcemanager.ha.admin.address</name>
+    <value>${yarn.resourcemanager.hostname}:8034</value>
...
+  private AccessControlList adminAcl;
+  private Server haAdminServer;
{code}

instanceof check before creating new conf?
{code}
+  public void setConf(Configuration conf) {
+    if (conf != null) {
+      conf = new YarnConfiguration(conf);
+    }
+    super.setConf(conf);
{code}

More clear if we name the parameter rmId instead of nodeId
{code}
protected HAServiceTarget resolveTarget(String nodeId) {
{code}

cast not needed right?
{code}
+    try {
+      return new RMHAServiceTarget((YarnConfiguration)getConf(), nodeId);
{code}

Should this create its own copy of the conf instead of overriding the CLI's 
copy? If its ok to override then we should probably do it in the CLI itself 
instead of being the side effect of a constructor.
{code}
public RMHAServiceTarget(YarnConfiguration conf, String nodeId)
+      throws IOException {
+    conf.set(YarnConfiguration.RM_HA_ID, nodeId);
{code}

After this how is someone supposed to get the socker addr? conf.getSocketAddr() 
will do the HAUtil magic instead of directly picking what is set by this code?
{code}
+    haAdminServer.start();
+    conf.updateConnectAddr(YarnConfiguration.RM_HA_ADMIN_ADDRESS,
+        haAdminServer.getListenerAddress());
{code}

Good to refactor. Looks like we should change AdminService to use the same 
method. Also, this method returns a user value that is used by AdminService to 
do audit logging. Would be good to follow that pattern and do audit logging in 
HAService at least for the state transition operations. probably not for the 
health check operations.
{code}
+    try {
+      RMServerUtils.verifyAccess(adminAcl, method, LOG);
{code}

Any notes on testing. There doesnt seem to be any new unit tests added.

> Add admin support for HA operations
> -----------------------------------
>
>                 Key: YARN-1068
>                 URL: https://issues.apache.org/jira/browse/YARN-1068
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>    Affects Versions: 2.1.0-beta
>            Reporter: Karthik Kambatla
>            Assignee: Karthik Kambatla
>              Labels: ha
>         Attachments: yarn-1068-1.patch, yarn-1068-2.patch, yarn-1068-3.patch, 
> yarn-1068-4.patch, yarn-1068-5.patch, yarn-1068-6.patch, yarn-1068-7.patch, 
> yarn-1068-8.patch, yarn-1068-9.patch, yarn-1068-prelim.patch
>
>
> Support HA admin operations to facilitate transitioning the RM to Active and 
> Standby states.



--
This message was sent by Atlassian JIRA
(v6.1#6144)

Reply via email to