> 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
> 
>

Reply via email to