You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jason Lai <ja...@jasonlai.net> on 2018/03/05 07:31:36 UTC

Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

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

Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.


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


Repository: mesos


Description
-------

Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
and replaced with those in a `MesosContainerLauncherHelper` subclass
instead.


Diffs
-----

  src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 


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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

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



Patch looks great!

Reviews applied: [65811, 65812, 65898, 65899]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On March 5, 2018, 7:31 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 5, 2018, 7:31 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65899/#review199276
-----------------------------------------------------------


Ship it!




Ship It!

- Eric Chung


On March 15, 2018, 8:18 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 15, 2018, 8:18 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65899/
-----------------------------------------------------------

(Updated March 15, 2018, 8:18 a.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.


Changes
-------

Use unique pointer over raw pointer of `MesosContainerizerLaunchHelper` instance.


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


Repository: mesos


Description
-------

Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
and replaced with those in a `MesosContainerLauncherHelper` subclass
instead.


Diffs (updated)
-----

  src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 


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

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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

Posted by Jason Lai <ja...@jasonlai.net>.

> On March 7, 2018, 2:28 a.m., Eric Chung wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 583 (patched)
> > <https://reviews.apache.org/r/65899/diff/1/?file=1970044#file1970044line824>
> >
> >     why is the explicit delete needed here? was it not being cleaned up previously?

A `MesosContainerizerLauncherHelper` instance was initiated as a raw pointer to the heap. So the intention was that the receiver takes care of managing the instance's lifecycle.

However, I think that we can have something smarter like wrapping the raw pointer with libprocess' `Owned` as a uniquely owned pointer. Will do that with a revision.


- Jason


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


On March 5, 2018, 7:31 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 5, 2018, 7:31 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65899/#review198759
-----------------------------------------------------------




src/slave/containerizer/mesos/launch.cpp
Lines 583 (patched)
<https://reviews.apache.org/r/65899/#comment278999>

    why is the explicit delete needed here? was it not being cleaned up previously?


- Eric Chung


On March 5, 2018, 7:31 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 5, 2018, 7:31 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

Posted by Jason Lai <ja...@jasonlai.net>.

> On March 5, 2018, 9:10 a.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['65811', '65812', '65898', '65899']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65899
> > 
> > Relevant logs:
> > 
> > - [mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65899/logs/mesos-tests-cmake-stdout.log):
> > 
> > ```
> >          D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(44): warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(45): warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(95): warning C4267: '=': conversion from 'size_t' to 'jint', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(463): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(493): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(654): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(693): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(770): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> >          D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(809): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
> > 
> > 
> >        "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
> >        "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (11) ->
> >        "D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj" (default target) (19) ->
> >        (Link target) -> 
> >          mesos.lib(launch.obj) : error LNK2019: unresolved external symbol "public: static class Try<class mesos::internal::slave::MesosContainerizerLaunchHelper *,class Error> __cdecl mesos::internal::slave::MesosContainerizerLaunchHelper::create(class mesos::slave::ContainerLaunchInfo const &)" (?create@MesosContainerizerLaunchHelper@slave@internal@mesos@@SA?AV?$Try@PEAVMesosContainerizerLaunchHelper@slave@internal@mesos@@VError@@@@AEBVContainerLaunchInfo@24@@Z) referenced in function "protected: virtual int __cdecl mesos::internal::slave::MesosContainerizerLaunch::execute(void)" (?execute@MesosContainerizerLaunch@slave@internal@mesos@@MEAAHXZ) [D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj]
> >          D:\DCOS\mesos\src\mesos-containerizer.exe : fatal error LNK1120: 1 unresolved externals [D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj]
> > 
> > 
> >        "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
> >        "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (11) ->
> >        "D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj" (default target) (29) ->
> >          mesos.lib(launch.obj) : error LNK2019: unresolved external symbol "public: static class Try<class mesos::internal::slave::MesosContainerizerLaunchHelper *,class Error> __cdecl mesos::internal::slave::MesosContainerizerLaunchHelper::create(class mesos::slave::ContainerLaunchInfo const &)" (?create@MesosContainerizerLaunchHelper@slave@internal@mesos@@SA?AV?$Try@PEAVMesosContainerizerLaunchHelper@slave@internal@mesos@@VError@@@@AEBVContainerLaunchInfo@24@@Z) referenced in function "protected: virtual int __cdecl mesos::internal::slave::MesosContainerizerLaunch::execute(void)" (?execute@MesosContainerizerLaunch@slave@internal@mesos@@MEAAHXZ) [D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj]
> >          D:\DCOS\mesos\src\mesos-executor.exe : fatal error LNK1120: 1 unresolved externals [D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj]
> > 
> >     214 Warning(s)
> >     4 Error(s)
> > 
> > Time Elapsed 00:23:35.74
> > ```

[65900](https://reviews.apache.org/r/65900/) has the latest test results for Windows and the 4 errors have been fixed.


- Jason


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


On March 5, 2018, 7:31 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 5, 2018, 7:31 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65899: Use launch actions in `MesosContainerizerLaunchHelper` instead.

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['65811', '65812', '65898', '65899']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65899

Relevant logs:

- [mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65899/logs/mesos-tests-cmake-stdout.log):

```
         D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(44): warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_state_Variable.cpp(45): warning C4267: 'argument': conversion from 'size_t' to 'jsize', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(95): warning C4267: '=': conversion from 'size_t' to 'jint', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(463): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(493): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(654): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(693): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(770): warning C4244: 'initializing': conversion from 'jchar' to 'char', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]
         D:\DCOS\mesos\mesos\src\jvm\jvm.cpp(809): warning C4244: 'initializing': conversion from 'jlong' to 'long', possible loss of data [D:\DCOS\mesos\src\java\mesos-java.vcxproj]


       "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
       "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (11) ->
       "D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj" (default target) (19) ->
       (Link target) -> 
         mesos.lib(launch.obj) : error LNK2019: unresolved external symbol "public: static class Try<class mesos::internal::slave::MesosContainerizerLaunchHelper *,class Error> __cdecl mesos::internal::slave::MesosContainerizerLaunchHelper::create(class mesos::slave::ContainerLaunchInfo const &)" (?create@MesosContainerizerLaunchHelper@slave@internal@mesos@@SA?AV?$Try@PEAVMesosContainerizerLaunchHelper@slave@internal@mesos@@VError@@@@AEBVContainerLaunchInfo@24@@Z) referenced in function "protected: virtual int __cdecl mesos::internal::slave::MesosContainerizerLaunch::execute(void)" (?execute@MesosContainerizerLaunch@slave@internal@mesos@@MEAAHXZ) [D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj]
         D:\DCOS\mesos\src\mesos-containerizer.exe : fatal error LNK1120: 1 unresolved externals [D:\DCOS\mesos\src\slave\containerizer\mesos\mesos-containerizer.vcxproj]


       "D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
       "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (11) ->
       "D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj" (default target) (29) ->
         mesos.lib(launch.obj) : error LNK2019: unresolved external symbol "public: static class Try<class mesos::internal::slave::MesosContainerizerLaunchHelper *,class Error> __cdecl mesos::internal::slave::MesosContainerizerLaunchHelper::create(class mesos::slave::ContainerLaunchInfo const &)" (?create@MesosContainerizerLaunchHelper@slave@internal@mesos@@SA?AV?$Try@PEAVMesosContainerizerLaunchHelper@slave@internal@mesos@@VError@@@@AEBVContainerLaunchInfo@24@@Z) referenced in function "protected: virtual int __cdecl mesos::internal::slave::MesosContainerizerLaunch::execute(void)" (?execute@MesosContainerizerLaunch@slave@internal@mesos@@MEAAHXZ) [D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj]
         D:\DCOS\mesos\src\mesos-executor.exe : fatal error LNK1120: 1 unresolved externals [D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj]

    214 Warning(s)
    4 Error(s)

Time Elapsed 00:23:35.74
```

- Mesos Reviewbot Windows


On March 5, 2018, 7:31 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65899/
> -----------------------------------------------------------
> 
> (Updated March 5, 2018, 7:31 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
>     https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed launch actions in `src/slave/containerizer/mesos/launch.cpp`
> and replaced with those in a `MesosContainerLauncherHelper` subclass
> instead.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp 75b7eaf9cd62d6b5f02896175168b651f4517e12 
> 
> 
> Diff: https://reviews.apache.org/r/65899/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>