[
https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16135306#comment-16135306
]
Eric Badger commented on YARN-6623:
-----------------------------------
[~vvasudev], I started reviewing the .003 patch, but I see that you've updated
a few times since then. So I'll post my comments so far and then continue my
review with the most recent patch.
Ran test-container-executor on both my mac and on rhel6 and it passed.
1. Should {{feature.docker.enabled}} be moved to be with the rest of the docker
configs?
2. For the Docker configs, I think it'd be more clear if they were prefixed
with {{docker.}}
3. Since we're going to remove the hard-coded string in YARN-6968, should we
leave the findbugs warning and take care of it there?
4.
{noformat}
+ /**
+ * Add command commandWithArguments - this method is only meant for use by
+ * sub-classes.
+ *
{noformat}
This is no longer just adding a command. Looks like it adds arguments to the
command and adds the command as well if it doesn't exist. So the comment should
be updated.
5.
{noformat}
+ this.commandArguments = new TreeMap<>();
+ commandArguments.put(dockerCommandKey, new ArrayList<>());
+ commandArguments.get(dockerCommandKey).add(command);
{noformat}
I’m not sure I understand how this piece of code is supposed to work. In this
method we add a new list with the key “docker-command”. But then in
{{addCommandArguments()}} we add arguments via the {{key}}, which is always
called in the subclasses with the name of the command being run (e.g. inspect).
So the map for each {{DockerCommand}} will usually have 2 keys,
“docker-command” and the command that actually needs arguments “inspect”. Is
this how it’s intended to work or did I miss something here?
6.
{noformat}
+ protected final void addCommandArguments(String key, String value) {
+ if (commandArguments.containsKey(key)) {
+ commandArguments.get(key).add(value);
+ return;
+ }
{noformat}
No need to call {{contains()}} and then call {{get()}}. We can just call get
and then add the value if the return value isn't null.
7.
{noformat}
+ @Override
+ public String toString() {
+ StringBuffer ret = new StringBuffer("");
+ ret.append(this.command);
{noformat}
Any reason not to create the string with the command in the constructor?
Something like
{{StringBuffer ret = new StringBuffer(this.command);}}
> Add support to turn off launching privileged containers in the
> container-executor
> ---------------------------------------------------------------------------------
>
> Key: YARN-6623
> URL: https://issues.apache.org/jira/browse/YARN-6623
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Priority: Blocker
> Attachments: YARN-6623.001.patch, YARN-6623.002.patch,
> YARN-6623.003.patch, YARN-6623.004.patch, YARN-6623.005.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should
> add a flag to the container-executor.cfg allowing admins to disable launching
> privileged containers at the container-executor level.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
