mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.
Date Mon, 16 Jan 2017 19:33:43 GMT

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



Looks good for the most part! Mostly minor cleanup comments.


src/slave/http.cpp (line 2184)
<https://reviews.apache.org/r/55464/#comment233051>

    Not yours, but we should rename this to `getExecutor(...)` to be consistent with other
similar functions in the same file.
    
    Worth doing in an earlier review with this review as dependent on it.



src/slave/http.cpp (line 2186)
<https://reviews.apache.org/r/55464/#comment233057>

    hmm, it looks error prone to look up frameworks only when an executor could be found and
then do error checking on L2192. It was fine to do so earlier since the information was being
populated while looping at one place. 
    
    How about:
    ```cpp
    Executor* executor = slave->getExecutor(containerId);
    if (executor == nullptr) {
      return NotFound("Unable to locate executor for container"
          " " + stringify(containerId));
    }
    
    Framework* framework = slave->getFramework(executor->frameworkId);
    CHECK_NOTNULL(framework);
    ```
    
    Note that the following invariant is always true that is an executor is *found*, it would
be associated with a framework.



src/slave/http.cpp (lines 2190 - 2195)
<https://reviews.apache.org/r/55464/#comment233056>

    hmm, why return a `BadRequest` here? This looks inconsistent with other instances where
we return `NotFound`. It does not seem that there was anything malformed with the client request.
This can also happen when the nested container exited right before the request was received
on the agent.
    
    Also, can we make the other error messages consistent with this?
    (See the snippet I posted in the previous comment)



src/slave/slave.cpp (lines 6227 - 6236)
<https://reviews.apache.org/r/55464/#comment233065>

    hmm, can we somehow reuse the helper `getRootContainerId()` in `slave/containerizer/mesos/utils.hpp`?
    
    You might have to pull it out though first and then re-use it in this review? Strictly
speaking, this method is pretty generic and should not be a part of the Mesos containerizer
code.



src/tests/api_tests.cpp (line 3619)
<https://reviews.apache.org/r/55464/#comment233070>

    Why did you change the existing test `NestedContainerLaunch` ? It's not immediately clear
from the diff. Can you instead create a review that just deletes the `NestedContainerLaunchGrandchildNotSupported`
test and then this review adds this new test. This might help RB not get confused with the
diff.



src/tests/api_tests.cpp (line 3740)
<https://reviews.apache.org/r/55464/#comment233072>

    How about `TwoLevelNestedContainerLaunch` instead?



src/tests/api_tests.cpp (line 3791)
<https://reviews.apache.org/r/55464/#comment233075>

    // Launch a two level nested parent/child container and then wait for them to finish.



src/tests/api_tests.cpp (line 3818)
<https://reviews.apache.org/r/55464/#comment233076>

    s/child child/child



src/tests/api_tests.cpp (line 3836)
<https://reviews.apache.org/r/55464/#comment233078>

    s/parent/parent container.



src/tests/api_tests.cpp (line 3838)
<https://reviews.apache.org/r/55464/#comment233077>

    newline before.



src/tests/api_tests.cpp (line 3852)
<https://reviews.apache.org/r/55464/#comment233079>

    s/child/child container.



src/tests/api_tests.cpp (line 3854)
<https://reviews.apache.org/r/55464/#comment233080>

    newline before


- Anand Mazumdar


On Jan. 13, 2017, 1:52 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55464/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2017, 1:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, Alexander Rojas,
and haosdent huang.
> 
> 
> Bugs: MESOS-6864
>     https://issues.apache.org/jira/browse/MESOS-6864
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made the Agent API able to handle containers nested at arbitrary levels.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 24fc23b229c624835a24cdda9587c99c6ac9c3bb 
>   src/slave/slave.cpp 11e8833fd5998abb71a7bb08e5dec451d894aba9 
>   src/tests/api_tests.cpp ea62226fbc5dd0bcc5ea60d1bbcd3748f9367467 
> 
> Diff: https://reviews.apache.org/r/55464/diff/
> 
> 
> Testing
> -------
> 
> `GTEST_FILTER="" make -j 8 check` in macOS.
> `make check` in Linux.
> 
> I also did some manual testing using a proof of concept  that makes the `DefaultExecutor`
leverage this change to perform CMD health checks.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


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