> 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. > > Ben Mahler wrote: > 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 :)
I think it's okay to have just a protobuf field, as long as we guarantee that it's always set to a sane value. - Alexander ----------------------------------------------------------- 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 > >
