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