mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 52473: Added nested MesosContainerizer tests.
Date Mon, 03 Oct 2016 21:30:14 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52473/#review151184
-----------------------------------------------------------




src/tests/containerizer/mesos_containerizer_tests.cpp (line 111)
<https://reviews.apache.org/r/52473/#comment219462>

    Can we set local == true consistently in tests if possible. It's much easier to debug
if test fails.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 48)
<https://reviews.apache.org/r/52473/#comment219464>

    We no longer use 'using namspace' in our code base. Please use 'using process::XXX' explictly.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 426)
<https://reviews.apache.org/r/52473/#comment219476>

    dash (default shell on ubuntu) does not support double digit file descriptor. This is
test may be flaky on some linux distributions. We should force to use 'bash' which does not
have this issue.
    https://bugs.launchpad.net/ubuntu/+source/dash/+bug/249620
    
    (BTW, we ran into this issue during debugging and I spent on like 4 hours debugging this
issue)
    
    To force to use 'bash', you need to use the non shell version of CommandInfo:
    ```
    shell=false
    value=bash
    arguments=['bash', '-c', 'read key <&x']
    ```



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 510 - 516)
<https://reviews.apache.org/r/52473/#comment219477>

    Aha, this is correct :)
    
    I think we may want to factor out this into createCommandInfo helper to support non shell
version.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 662)
<https://reviews.apache.org/r/52473/#comment219478>

    Why reset here given you'll reset below? Please fix all other occurances.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 750 - 752)
<https://reviews.apache.org/r/52473/#comment219480>

    I don't like the current way of simulating a launcher orphan. First, it's linux launcher
specific. It uses some internal impl. details in the linux launcher (painful to update when
that layout changes). In the future, I think we should still parameterize the tests in this
file to use both posix launcher or linux launcher.
    
    To simulate a launcher orphan, i think we should launch container, and then wipe the runtime
dir.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 942 - 943)
<https://reviews.apache.org/r/52473/#comment219536>

    I'd prefer getting `containerizer->wait` to the next line.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 955 - 956)
<https://reviews.apache.org/r/52473/#comment219537>

    Instead of calling 'status' to test if a container has gone or not. I'd actually prefer
we call 'containers' and test if containerId belongs to the returned hashset.
    
    This is because 'status' can fail in different ways, not just because the container does
not exist.


- Jie Yu


On Oct. 3, 2016, 3:28 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52473/
> -----------------------------------------------------------
> 
> (Updated Oct. 3, 2016, 3:28 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added nested MesosContainerizer tests.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am c897d863bde284de41c99ed50ccbbdfd2dcad23b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 8b5a19e121ef74eaf99b39682f8fd170605bf56d

>   src/tests/containerizer/nested_container_tests.cpp 8430823d54d132bc3e9c3e0781f027072999683e

>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52473/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message