> On Feb. 5, 2015, 12:07 a.m., Niklas Nielsen wrote: > > src/slave/containerizer/containerizer.cpp, line 237 > > <https://reviews.apache.org/r/30583/diff/2/?file=849006#file849006line237> > > > > Wouldn't it simplify executorEnvironment() if we set the default grace > > period in getExecutorInfo() (and thereby _guaranteeing_ that the executor's > > command info always have a grace period set?) We do this for the default > > container. > > > > Know we talked about this before, but forgot whether we got to a > > conclusion. > > Ben Mahler wrote: > It doesn't look like that gives us much simplification here. > > I'm a bit hesitant to mutate a framework's ExecutorInfo because then you > can no longer distinguish between what is specified by the framework and what > was set using slave defaults. Maybe we'll want to start filling things in and > allowing the downstream components to assume things are set. I don't have the > foresight but my intuition is warning me that it will be a path that is hard > to return from if we realize we need to distinguish. > > Niklas Nielsen wrote: > If the executor info is guaranteed to have grace period set, you would > only need a CHECK in executorEnvironment() and you could drop exposing the > entire slave flags object, but I may be missing something? > The source of truth (to me) should be in the command info instead of > having to have the two values following eachother in parallel. It simlifies > the control flow of the downstream logic and make it less errorprone when > introducing new code. Think it's a matter of taste - capturing it in the data > or control flow; can't go to an extreme, but setting it immediately seems > cleaner to me. If you don't think it will help - feel free to ignore the > comment.
What you're saying is totally valid, although I think that it could have been done by applying a default directly on the optional protobuf field. The approach of injecting the information from a slave flag doesn't seem like the best way to go. I think we should consider getting rid of the slave flag entirely now that it's been added to CommandInfo, and just rely on the default in the protobuf. Have you guys considered that? Either way, I'll let you and Alex follow up :) - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30583/#review71067 ----------------------------------------------------------- On Feb. 4, 2015, 10:43 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30583/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2015, 10:43 p.m.) > > > Review request for mesos, Alexander Rukletsov, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-2228 > https://issues.apache.org/jira/browse/MESOS-2228 > > > Repository: mesos > > > Description > ------- > > Now the `"MESOS_RECOVERY_TIMEOUT"` environment variable is set based on the > flag value, instead of the `EXECUTOR_SHUTDOWN_GRACE_PERIOD` constant. > > It also now correctly uses `Duration::create` to interpret the protobuf value. > > > Diffs > ----- > > src/exec/exec.cpp aada24664dba9060a92230e25689c89852585443 > src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504 > src/slave/containerizer/containerizer.hpp > 129e60f20835f5d151701e934330b81825887af1 > src/slave/containerizer/containerizer.cpp > 421bb868b353e644578fa27f04bdd636bfc89134 > src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 > src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7 > src/slave/containerizer/external_containerizer.cpp > 8d5a9047afb24a29413bfc7226f47b1edbfa4ff9 > src/slave/containerizer/mesos/containerizer.cpp > d712278428889ebdfd598537690138329d8464f0 > src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 > src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 > src/tests/gc_tests.cpp 454f0974833ad5db8b504a36b010cc72c3a19751 > src/tests/hook_tests.cpp 44f73effdce2d03627215418007ccbc3263a0c52 > src/tests/master_allocator_tests.cpp > 018a6ccba8bb78974de06398f423f1f30ef3a3df > src/tests/master_tests.cpp 678d27f41a2f246c714c77adb132263c0c2c61ed > src/tests/mesos.cpp 5ed4df530cf1bf11eec3b29542641822e0f702b2 > src/tests/slave_recovery_tests.cpp 7e2e63d4b8a1cd7c191374bc37073d83ae413e03 > src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6 > > Diff: https://reviews.apache.org/r/30583/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
