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 58903: Combined Mesos containerizer's launch methods.
Date Thu, 11 May 2017 12:32:33 GMT

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


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp
Lines 984-986 (original), 931-933 (patched)
<https://reviews.apache.org/r/58903/#comment247834>

    In fact, I would call the parameter `_containerConfig` here:
    
    ```
    ContainerConfig containerConfig = _containerConfig;
    ```
    
    And moving forward, consistently use `containerConfig` in this function.



src/slave/containerizer/mesos/containerizer.cpp
Line 1066 (original), 1037 (patched)
<https://reviews.apache.org/r/58903/#comment247832>

    It's weird that you still `containerConfig` not `containerConfig_` here.
    
    See my comments above.


- Jie Yu


On May 2, 2017, 2:04 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58903/
> -----------------------------------------------------------
> 
> (Updated May 2, 2017, 2:04 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7449
>     https://issues.apache.org/jira/browse/MESOS-7449
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This simplifies the container launch path by removing combining
> the nested and non-nested container code paths into one.
> 
> The Mesos containerizer was originally translating the two
> `containerizer->launch` entrypoints into a common method (also
> called `launch`).  The previous commits moved this translation
> logic into the caller (i.e. the Agent).
> 
> The end result has some slight changes:
>   * It is now possible for the Agent to specify some more combinations
>     of `ContainerConfig`.  For example, specifying a TaskInfo with
>     a DEBUG-class container.  Or a nested container with Resources.
>     We may need to add extra validation around this.
>   * The `bool checkpoint` argument was replaced with a `Option<string>`
>     which optionally contains an absolute path.  This allows us to
>     remove the `SlaveID` field.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d

>   src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae

> 
> 
> Diff: https://reviews.apache.org/r/58903/diff/1/
> 
> 
> Testing
> -------
> 
> See last patch in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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