hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Joseph Evans (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-186) Coverage fixing LinuxContainerExecuror
Date Thu, 01 Nov 2012 15:39:14 GMT

    [ https://issues.apache.org/jira/browse/YARN-186?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13488769#comment-13488769

Robert Joseph Evans commented on YARN-186:

The tests look good. but I have a few comments.

# There are also two existing tests for the LinuxContainerExecutor.  TestLinuxContainerExecutor
is more of an integration test and requires some manual setup with the actual binary to make
it work.  TestLinuxContainerExecutorWithMocks tries to test the code by mocking out parts
under it.  The tests here look like they should be added into TestLinuxContainerExecutorWithMocks
instead of adding in a new file.
# In setup() a LinuxContainerExecutor exec is created, but it is only used in one of the tests,
testErrorLauncher.  It would probably be better to have the initialization happen in the test
instead, which if you combine with TestLinuxContainerExecutorWithMocks this becomes a bit
of a noop, because it is doing almost the exact same thing already for other tests.
# At a minimum some documentation about what writeScriptFile does would be good.  Having it
write a script that will echo the arguments into a file named after the last argument seems
kind of confusing.  Especially when to read those arguments you need to open a file named
'--checksetup'. I personally would like to see it write the arguments out to a consistently
named file, so that if the arguments are wrong you get an assertion indicating that instead
of a FileNotFoundException.  Which again if you combine with TestLinuxContainerExecutorWithMocks
the writeScriptFile goes away because there is already code in the test to do something very
# I would also like to see testInitException explained a little bit better what it is trying
to test.  It is confusing why only setting the executor path to /bin/sh causes init() to fail
but in setup() you are setting it to /bin/bash, and you skip calling the init() method.

Also if your patches are identical you don't really need to post separate patches for each
branch.  One patch for trunk that applies to all of the branches is simples to review.

> Coverage fixing LinuxContainerExecuror
> --------------------------------------
>                 Key: YARN-186
>                 URL: https://issues.apache.org/jira/browse/YARN-186
>             Project: Hadoop YARN
>          Issue Type: Test
>          Components: resourcemanager, scheduler
>            Reporter: Aleksey Gorshkov
>         Attachments: YARN-186-branch-0.23--a.patch, YARN-186-branch-0.23.patch, YARN-186-branch-2--a.patch,
YARN-186-branch-2.patch, YARN-186-trunk--a.patch, YARN-186-trunk.patch
> Added some tests for LinuxContainerExecuror  
> YARN-186-branch-0.23.patch patch for branch-0.23
> YARN-186-branch-2.patch patch for branch-2
> ARN-186-trunk.patch patch for trank

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

View raw message