[
https://issues.apache.org/jira/browse/YARN-6374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15985754#comment-15985754
]
Sidharta Seethana commented on YARN-6374:
-----------------------------------------
[[email protected]], thanks for the updated patch. Here is some feedback :
{code}
dockerOp.disableFailureLogging();
{code}
Why is failure logging disabled for all operations that use this method? So far
this has only been the case for signaling containers.
{code}
LOG.debug("Running docker rm. ContainerId: " + containerId);
{code}
Could you enclose such debug log lines with checks to see if debug logging is
enabled?
{code}
public static void executeDockerRm(String containerId,
{code}
I see the value in {{executeDockerCommand}} - but the other methods (e.g
{{executeDockerRm}}) might not actually be beneficial. Instead of
{{DockerCommandExecutor.executeDockerRm(containerId, …)}}, it would be
{{DockerCommandExecutor.executeDockerCommand(new DockerRmCommand(containerId),
... )}} . I am not sure there is much point to these additional wrappers. In
fact, this would mean additional work every time a new subclass of
{{DockerCommand}} is added.
{code}
public DockerContainerStatus getContainerStatus(String containerId,
Configuration conf,
PrivilegedOperationExecutor privilegedOperationExecutor) {
{code}
Is there a reason this method isn’t static like the utility methods in the
{{DockerCommandExecutor}} class? What do you think about simplifying this by
combining both of the these into one class (i.e is there a reason
{{getContainerStatus}} can’t be bundled with {{DockerCommandExecutor}} ?)
{code}
public DockerLinuxContainerRuntime(PrivilegedOperationExecutor
privilegedOperationExecutor, CGroupsHandler cGroupsHandler,
DockerContainerStatusHandler dockerContainerStatusHandler) {
{code}
So far, everything docker related has been encapsulated within the runtime
class ( and this includes the {{DockerClient}} which {{DockerCommandExecutor}}
uses). It seems counter-intuitive to inject something as specific as a
{{DockerContainerStatusHandler}} as a dependency for
{{DockerLinuxContainerRuntime}} itself.
{code}
public class MockDockerContainerStatusHandler
extends DockerContainerStatusHandler {
.
.
.
protected String executeStatusCommand(String containerId,
Configuration conf,
PrivilegedOperationExecutor privilegedOperationExecutor)
throws ContainerExecutionException {
{code}
What would be the reason to execute the inspect if the objective is return a
mocked/injected status?
{code}
@Test
public void testExecuteDockerRm() throws Exception {
DockerCommandExecutor
{code}
Same question as earlier - is it necessary to add all these wrappers?
{code}
@Test
public void testGetContainerStatusForCreated()
{code}
There seem to be a lot of these test methods to test one if-else tree in
{{getContainerStatus}}. Wouldn’t it easier to list all the distinct enum values
and test this in a loop as opposed to writing 8 functions?
{code}
@Test
public void testRemoveContainerOnExit() throws Exception {
{code}
Lot of new tests here to test argument concatenation in the
{{DockerRunCommand}} class - do you think these can be consolidated into fewer
methods? Some of this is already tested in the {{TestDockerContainerRuntime}}
class.
> Improve test coverage and add utility classes for common Docker operations
> --------------------------------------------------------------------------
>
> Key: YARN-6374
> URL: https://issues.apache.org/jira/browse/YARN-6374
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, yarn
> Reporter: Shane Kumpf
> Assignee: Shane Kumpf
> Attachments: YARN-6374.001.patch, YARN-6374.002.patch
>
>
> Currently, it is tedious to execute Docker related operations due to the
> plumbing needed to define the DockerCommand, writing the command file,
> configuring privileged operation, and finally executing the command and
> validating the result. Obtaining the current status of a Docker container can
> also be improved. Finally, the test coverage is lacking for Docker Commands.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
