You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/08/08 09:44:29 UTC

Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

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

Review request for mesos.


Repository: mesos


Description
-------

Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.


Diffs
-----

  src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 


Diff: https://reviews.apache.org/r/61493/diff/1/


Testing
-------


Thanks,

Qian Zhang

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182880
-----------------------------------------------------------


Ship it!




Ship It!

- Gastón Kleiman


On Aug. 14, 2017, 2:06 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 2:06 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182916
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Line 1375 (original), 1375 (patched)
<https://reviews.apache.org/r/61493/#comment258856>

    this test is linux specific, need `#ifdef __linux__`.



src/tests/default_executor_tests.cpp
Lines 1378 (patched)
<https://reviews.apache.org/r/61493/#comment258857>

    should be a `ROOT` test.



src/tests/default_executor_tests.cpp
Lines 1383-1384 (patched)
<https://reviews.apache.org/r/61493/#comment258858>

    `namespces/pid` isolator is not specified! all containers are sharing the agent's pid ns by default.



src/tests/default_executor_tests.cpp
Lines 1414-1419 (patched)
<https://reviews.apache.org/r/61493/#comment258859>

    not yours. this is a tech debt in this file. let's use `v1::scheduler::SendSubscribe()`.



src/tests/default_executor_tests.cpp
Lines 1427-1431 (patched)
<https://reviews.apache.org/r/61493/#comment258860>

    use `v1::createExecutorInfo()`



src/tests/default_executor_tests.cpp
Lines 1463-1466 (patched)
<https://reviews.apache.org/r/61493/#comment258861>

    oneline instead.



src/tests/default_executor_tests.cpp
Lines 1498-1501 (patched)
<https://reviews.apache.org/r/61493/#comment258862>

    ditto.



src/tests/default_executor_tests.cpp
Lines 1510-1514 (patched)
<https://reviews.apache.org/r/61493/#comment258863>

    save as `executorSandbox`.


- Gilbert Song


On Aug. 13, 2017, 7:06 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182837
-----------------------------------------------------------



Bad patch!

Reviews applied: [61270, 61406, 61428, 61463, 61464, 61465, 61466, 61493]

Logs available here: http://104.210.40.105/logs/master/61493

- Mesos Reviewbot Windows


On Aug. 14, 2017, 2:06 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 2:06 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/
-----------------------------------------------------------

(Updated Aug. 14, 2017, 10:06 a.m.)


Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
-------

Addressed comments.


Bugs: MESOS-7853
    https://issues.apache.org/jira/browse/MESOS-7853


Repository: mesos


Description
-------

Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.


Diffs (updated)
-----

  src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 


Diff: https://reviews.apache.org/r/61493/diff/2/

Changes: https://reviews.apache.org/r/61493/diff/1-2/


Testing
-------


Thanks,

Qian Zhang

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182770
-----------------------------------------------------------



Bad patch!

Reviews applied: [61270, 61406, 61428, 61463, 61464, 61465, 61466, 61493]

Logs available here: http://104.210.40.105/logs/master/61493

- Mesos Reviewbot Windows


On Aug. 8, 2017, 9:44 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 9:44 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Qian Zhang <zh...@gmail.com>.

> On Aug. 12, 2017, 6:38 a.m., Gilbert Song wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1580 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1580>
> >
> >     you may want to `strings::trim()`.

Good catch!


- Qian


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


On Aug. 14, 2017, 10:06 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182757
-----------------------------------------------------------




src/tests/default_executor_tests.cpp
Lines 1580 (patched)
<https://reviews.apache.org/r/61493/#comment258755>

    you may want to `strings::trim()`.


- Gilbert Song


On Aug. 8, 2017, 2:44 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 2:44 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Qian Zhang <zh...@gmail.com>.

> On Aug. 12, 2017, 9:31 a.m., Gastón Kleiman wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1451-1452 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1451>
> >
> >     What do you think about the following alternative?
> >     
> >     `v1::TaskGroupInfo taskGroup = v1::createTaskGroupInfo({taskInfo})`

+1


> On Aug. 12, 2017, 9:31 a.m., Gastón Kleiman wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1465-1481 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1465>
> >
> >     I think that the following is shorter and more readable:
> >     
> >     ```
> >     v1::Offer::Operation launchGroup = v1::LAUNCH_GROUP(
> >         executorInfo,
> >         v1::createTaskGroupInfo({taskInfo}));
> >         
> >     mesos.send(v1::createCallAccept(
> >         frameworkId,
> >         offer1,
> >         {launchGroup}));
> >     ```

+1


- Qian


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


On Aug. 14, 2017, 10:06 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182775
-----------------------------------------------------------




src/tests/default_executor_tests.cpp
Lines 1436 (patched)
<https://reviews.apache.org/r/61493/#comment258770>

    Please use:
    
    `EXPECT_FALSE(offers1->offers().empty());`



src/tests/default_executor_tests.cpp
Lines 1451-1452 (patched)
<https://reviews.apache.org/r/61493/#comment258772>

    What do you think about the following alternative?
    
    `v1::TaskGroupInfo taskGroup = v1::createTaskGroupInfo({taskInfo})`



src/tests/default_executor_tests.cpp
Lines 1465-1481 (patched)
<https://reviews.apache.org/r/61493/#comment258773>

    I think that the following is shorter and more readable:
    
    ```
    v1::Offer::Operation launchGroup = v1::LAUNCH_GROUP(
        executorInfo,
        v1::createTaskGroupInfo({taskInfo}));
        
    mesos.send(v1::createCallAccept(
        frameworkId,
        offer1,
        {launchGroup}));
    ```



src/tests/default_executor_tests.cpp
Lines 1491 (patched)
<https://reviews.apache.org/r/61493/#comment258771>

    Add
    
    `EXPECT_FALSE(offers2->offers().empty());`



src/tests/default_executor_tests.cpp
Lines 1503-1504 (patched)
<https://reviews.apache.org/r/61493/#comment258774>

    Ditto.



src/tests/default_executor_tests.cpp
Lines 1511-1529 (patched)
<https://reviews.apache.org/r/61493/#comment258775>

    Ditto.



src/tests/default_executor_tests.cpp
Lines 1547 (patched)
<https://reviews.apache.org/r/61493/#comment258776>

    Remove this extra line.


- Gastón Kleiman


On Aug. 8, 2017, 9:44 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 9:44 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Qian Zhang <zh...@gmail.com>.

> On Aug. 12, 2017, 6:27 a.m., Gilbert Song wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1538-1542 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1538>
> >
> >     Could we save the executor sandbox to a var and call `getSandboxPath()`?

`getSandboxPath()` needs `containerId` as one of its inputs, but in this test, we do not have containerId.


> On Aug. 12, 2017, 6:27 a.m., Gilbert Song wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1554 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1554>
> >
> >     we should not rely on the symlink to get the nested sandbox.

Can you elaborate why?


> On Aug. 12, 2017, 6:27 a.m., Gilbert Song wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1558-1568 (patched)
> > <https://reviews.apache.org/r/61493/diff/1/?file=1791804#file1791804line1558>
> >
> >     why do we need these? Could we do `AWAIT_TRUE()`?

We can not do `AWAIT_TRUE()` because it needs a `Future` as its input, but in this test we do not have that. And you can take a look at the test `CpuIsolatorTest.ROOT_UserCpuUsage` which does the same way.


- Qian


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


On Aug. 14, 2017, 10:06 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 3ac9f292f928aa67ee380a63233832365daa2cc2 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/#review182755
-----------------------------------------------------------




src/tests/default_executor_tests.cpp
Lines 1538-1542 (patched)
<https://reviews.apache.org/r/61493/#comment258750>

    Could we save the executor sandbox to a var and call `getSandboxPath()`?



src/tests/default_executor_tests.cpp
Lines 1554 (patched)
<https://reviews.apache.org/r/61493/#comment258753>

    we should not rely on the symlink to get the nested sandbox.



src/tests/default_executor_tests.cpp
Lines 1558-1568 (patched)
<https://reviews.apache.org/r/61493/#comment258752>

    why do we need these? Could we do `AWAIT_TRUE()`?


- Gilbert Song


On Aug. 8, 2017, 2:44 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61493/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 2:44 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-7853
>     https://issues.apache.org/jira/browse/MESOS-7853
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 
> 
> 
> Diff: https://reviews.apache.org/r/61493/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61493: Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.

Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61493/
-----------------------------------------------------------

(Updated Aug. 8, 2017, 5:44 p.m.)


Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.


Bugs: MESOS-7853
    https://issues.apache.org/jira/browse/MESOS-7853


Repository: mesos


Description
-------

Added a test `DefaultExecutorTest.MultiTaskgroupSharePidNamespace`.


Diffs
-----

  src/tests/default_executor_tests.cpp b9776314a8781963b92ba9ac297654f61a443bc8 


Diff: https://reviews.apache.org/r/61493/diff/1/


Testing
-------


Thanks,

Qian Zhang