mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.com>
Subject Re: Review Request 55464: Made the Agent API able to handle containers nested at arbitrary levels.
Date Wed, 18 Jan 2017 19:02:23 GMT


> On Jan. 16, 2017, 7:33 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2184
> > <https://reviews.apache.org/r/55464/diff/3/?file=1604722#file1604722line2184>
> >
> >     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.

https://reviews.apache.org/r/55678/


> On Jan. 16, 2017, 7:33 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2199
> > <https://reviews.apache.org/r/55464/diff/3/?file=1604722#file1604722line2199>
> >
> >     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.

https://reviews.apache.org/r/55679/


> On Jan. 16, 2017, 7:33 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, lines 2209-2214
> > <https://reviews.apache.org/r/55464/diff/3/?file=1604722#file1604722line2209>
> >
> >     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)

Also in https://reviews.apache.org/r/55679/


> On Jan. 16, 2017, 7:33 p.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, lines 6227-6236
> > <https://reviews.apache.org/r/55464/diff/3/?file=1604723#file1604723line6227>
> >
> >     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.

https://reviews.apache.org/r/55676/


- Gastón


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


On Jan. 18, 2017, 2:48 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55464/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2017, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, Alexander Rojas,
haosdent huang, and Vinod Kone.
> 
> 
> 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 ee7119179a6ddd935c3f43a618ef645619c305ee 
>   src/slave/slave.cpp ae183e6b8b40094b4764525a6d63164f210ef9d8 
>   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