[
https://issues.apache.org/jira/browse/YARN-9475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16818945#comment-16818945
]
Szilard Nemeth commented on YARN-9475:
--------------------------------------
Thanks [~pbacsko] for this patch!
Couple of comments:
1. The constructor
{{org.apache.hadoop.yarn.server.nodemanager.containermanager.resourceplugin.com.nec.NECVEPlugin#NECVEPlugin(java.util.function.Function<java.lang.String,java.lang.String>,
java.lang.String[])}} could be private, but as you said offline, you want to
use this from tests later, so this is a no-op.
2. IntelliJ complains about the {{binaryName}} / {{binaryFile}} fields: They
could be local variables as you only use them from inside the constructor. Do
you really need these as fields?
3. In the constructor, there's this:
{code:java}
if (null != envBinaryName) {
{code}
I think the null check is more readable on the other way around.
4. {{LOG.warn("Specified path is a directory, falling back");}} --> Log could
be improved to describe what we are falling back to.
5. Please extract methods, the constructor is 50 lines long, this is too much.
I can see at least 3 methods here:
- A method that reads the binary path based on the
{{NECVEPlugin#ENV_SCRIPT_PATH}}
- A method that reads the binary path based on the
{{NECVEPlugin#HADOOP_COMMON_HOME}}
- A method that reads {{binaryFile}} / {{binaryPath}} based on the
{{scriptPaths}} variable.
Btw, I can see that this.binaryPath is modified many times in the constructor:
Does the ordering of the code represents some kind of precendence on the env
variables?
6. In {{NECVEPlugin#getDevices}}: I can see no reason to declare the output
String outside of the if. Please declare + assign in one go inside the
try-catch block!
7. {{NECVEPlugin#onDevicesAllocated}} does not throw any Exception so the
signature should not contains a throws-clause.
8. In {{NECVEPlugin#parseOutput}}: keyvalues should be keyValues instead (note
the uppercase 'V').
9. In {{NECVEPlugin#parseOutput}}: The declaration of device could be moved to
the assignment.
10. In {{NECVEPlugin#parseOutput}}:
{{LOG.error("Unknown format of script output! Skip this line");}} --> Text
should start with "Skipping" instead.
11. In {{NECVEPlugin#parseOutput}}: There's a typo in this comment:
{code:java}
// for key value pars{code}
12. In {{NECVEPlugin#parseOutput}}: The long if-chain should be if-else for
every key, right? Assuming that for one key, we should not update the builder
twice.
Moreover, I would prefer to use a Map where you store keys as strings (like
"id", "dev", etc.) and Function object as values, that can act on the builder,
like invoking a set operation on it.
This way, the whole if-chain could be eliminated.
13. In {{NECVEPlugin#allocateDevices}}: The Device type parameter is not
required for the {{HashSet}}.
> Create basic VE plugin
> ----------------------
>
> Key: YARN-9475
> URL: https://issues.apache.org/jira/browse/YARN-9475
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Reporter: Peter Bacsko
> Assignee: Peter Bacsko
> Priority: Major
> Attachments: YARN-9475-001.patch, YARN-9475-002.patch,
> YARN-9475-003.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
