Allen Wittenauer commented on YARN-3484:

* variables that are local to a function should be declared local.  
* avoid using mixed case as per the shell programming guidelines
* yarnTopArgs is effectively a global.  It should either get renamed to 
YARN_foo or another  to not pollute the shell name space or another approach is 
process set_yarn_top_args as a subshell, reading its input directly to avoid 
the global entirely
* set_yarn_top_args should be hadoop_ something so as to not pollute the shell 
name space 
* nit: technically, TERM isn't guaranteed to be set on all OSes under all 
workable modes, since it is the login process' responsibility to set it.  
However, almost all modern systems do set it and it's fairly reliable. I think 
it's OK to leave the check, but I wanted to make this comment here for future 
readers in case they hit the situation where TERM wasn't set for their 
particular system.  Yes, that situation was thought about, but honestly, 

> Fix up yarn top shell code
> --------------------------
>                 Key: YARN-3484
>                 URL: https://issues.apache.org/jira/browse/YARN-3484
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: scripts
>    Affects Versions: 3.0.0
>            Reporter: Allen Wittenauer
>            Assignee: Varun Vasudev
>         Attachments: YARN-3484.001.patch
> We need to do some work on yarn top's shell code.
> a) Just checking for TERM isn't good enough.  We really need to check the 
> return on tput, especially since the output will not be a number but an error 
> string which will likely blow up the java code in horrible ways.
> b) All the single bracket tests should be double brackets to force the bash 
> built-in.
> c) I'd think I'd rather see the shell portion in a function since it's rather 
> large.  This will allow for args, etc, to get local'ized and clean up the 
> case statement.

This message was sent by Atlassian JIRA

Reply via email to