[
https://issues.apache.org/jira/browse/YARN-7815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16354171#comment-16354171
]
Jason Lowe commented on YARN-7815:
----------------------------------
Thanks for updating the patch! Looks good overall, just a few nits.
bq. I'm hesitant to fix these two check style issues as I don't believe it
improves the code by doing so, but I also don't like adding new check style
issues.
Agreed. The warning is useful when it isn't a setter function, as it can point
to a subtle bug. I wish checkstyle could automatically detect the difference
(e.g.: notice the function starts with 'set' and updates the hidden variable
with the eclipsing variable). In the meantime we can keep the code and
suppress the warning with:
{noformat}
@SuppressWarnings("checkstyle:hiddenfield")
{noformat}
Nit: In DockerContainerLinuxRuntime the array lists are sized with one
collection but then another is added. This requires a realloc of the original
array, and it would be nice to size it as the sum of all lists we intend to
concatenate to avoid the realloc. Actually is it worth creating a new array
list just so we can iterate it? We can iterate the two lists separately
without needing to create copies of them. A utility function that takes a list
and adds the mount command would help with readability if that's the concern.
Nit: Not directly related since this change was just refactoring existing code,
but the comment here says a copy is made of the environment. I don't see how
the copy occurs, it just modifies the map directly.
launchContext.getEnvironment does not return a new map each time. Either we do
not want/need the copy and the comment is wrong, or we are inadvertently
mangling the original map instead of a copy.
{code}
private Map<String, String> expandAllEnvironmentVars(
ContainerLaunchContext launchContext, Path containerLogDir) {
Map<String, String> environment = launchContext.getEnvironment();
// Make a copy of env to iterate & do variable expansion
for (Entry<String, String> entry : environment.entrySet()) {
String value = entry.getValue();
value = expandEnvironment(value, containerLogDir);
entry.setValue(value);
}
return environment;
}
{code}
> Make the YARN mounts added to Docker containers more restrictive
> ----------------------------------------------------------------
>
> Key: YARN-7815
> URL: https://issues.apache.org/jira/browse/YARN-7815
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Shane Kumpf
> Assignee: Shane Kumpf
> Priority: Major
> Attachments: YARN-7815.001.patch, YARN-7815.002.patch,
> YARN-7815.003.patch
>
>
> Currently, when using the Docker runtime, the filecache directories are
> mounted read-write into the Docker containers. Read write access is not
> necessary. We should make this more restrictive by changing that mount to
> read-only.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
