Junping Du commented on YARN-3226:

The patch LGTM in overall. Some NITs:
+    default :
+      LOG.debug("Unexpcted inital state");
+    default :
+      LOG.debug("Unexpcted final state");
We should have warn as log level because this is unexpected. Also, a typo here: 
"inital" => "initial".

-        metrics.incrDecommisionedNMs();
+    metrics.incrDecommisionedNMs();
May be indentation problems.

Also, I need someone to review UI changes. [~xgong], can you take a look at it? 

> UI changes for decommissioning node
> -----------------------------------
>                 Key: YARN-3226
>                 URL: https://issues.apache.org/jira/browse/YARN-3226
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: graceful
>            Reporter: Junping Du
>            Assignee: Sunil G
>         Attachments: 0001-YARN-3226.patch, 0002-YARN-3226.patch, 
> 0003-YARN-3226.patch, ClusterMetricsOnNodes_UI.png
> Some initial thought is:
> decommissioning nodes should still show up in the active nodes list since 
> they are still running containers. 
> A separate decommissioning tab to filter for those nodes would be nice, 
> although I suppose users can also just use the jquery table to sort/search for
> nodes in that state from the active nodes list if it's too crowded to add yet 
> another node
> state tab (or maybe get rid of some effectively dead tabs like the reboot 
> state tab).

This message was sent by Atlassian JIRA

Reply via email to