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