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

Junping Du commented on YARN-3225:
----------------------------------

Thanks [~devaraj.k] for the patch! The latest patch looks good in overall. 
Some minor comments:
{code}
+  private long validateTimeout(String strTimeout) {
+    long timeout;
+    try {
+      timeout = Long.parseLong(strTimeout);
+    } catch (NumberFormatException ex) {
+      throw new IllegalArgumentException(INVALID_TIMEOUT_ERR_MSG + strTimeout);
+    }
+    if (timeout < 0) {
+      throw new IllegalArgumentException(INVALID_TIMEOUT_ERR_MSG + timeout);
+    }
+    return timeout;
+  }
{code}
I think we should support a case that Admin want node get decommissioned 
whenever all apps on these node get finished. If so, shall we support nigative 
value (anyone or some special one, like: -1) to specify this case?

javadoc in DecommissionType.java
{code}
+  /** Decomissioning nodes **/
+  NORMAL,
+
+  /** Graceful decommissioning of nodes **/
+  GRACEFUL,
+
+  /** Forceful decommissioning of nodes **/
+  FORCEFUL
{code}
For NORMAL, shall we use "Decommission nodes in normal (old) way" instead or 
something simpler- "Decommission nodes"?

{code}
+@Private
+@Unstable
+public abstract class CheckForDecommissioningNodesRequest {
+  @Public
+  @Unstable
+  public static CheckForDecommissioningNodesRequest newInstance() {
+    CheckForDecommissioningNodesRequest request = Records
+        .newRecord(CheckForDecommissioningNodesRequest.class);
+    return request;
+  }
+}
{code}
IMO, the methods inside a class should't be more public than class itself? If 
we don't expect other projects to use class, we alwasy don't expect some 
methods get used. The same problem happen in an old API 
RefreshNodeRequest.java. I think we may need to fix both?

{code}
   @Test
   public void testRefreshNodes() throws Exception {
     resourceManager.getClientRMService();
-    RefreshNodesRequest request = recordFactory
-            .newRecordInstance(RefreshNodesRequest.class);
+    RefreshNodesRequest request = RefreshNodesRequest
+        .newInstance(DecommissionType.NORMAL);
     RefreshNodesResponse response = client.refreshNodes(request);
     assertNotNull(response);
   }
{code}
Why do we need this change? 
recordFactory.newRecordInstance(RefreshNodesRequest.class) will return 
something with DecommissionType.NORMAL as default. No?

> New parameter or CLI for decommissioning node gracefully in RMAdmin CLI
> -----------------------------------------------------------------------
>
>                 Key: YARN-3225
>                 URL: https://issues.apache.org/jira/browse/YARN-3225
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Junping Du
>            Assignee: Devaraj K
>         Attachments: YARN-3225-1.patch, YARN-3225-2.patch, YARN-3225-3.patch, 
> YARN-3225.patch, YARN-914.patch
>
>
> New CLI (or existing CLI with parameters) should put each node on 
> decommission list to decommissioning status and track timeout to terminate 
> the nodes that haven't get finished.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to