----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17567/#review37069 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/17567/#comment68330> I saw this was raised before, but why not just use External everywhere, i.e., ExternalContainerizer has an ExternalTermination, and rename files/classes src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/17567/#comment68333> ExternalContainerizer? ;-) src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68445> could you please clarify why we need this tuple? why do we need to wait on both - could we not .then on one before the other? src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68334> pid_t is a simple data type so just pass by value src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68335> no need for const on pass by value src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68339> use const references for list<> and Duration. no need for const on unsigned int src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68441> rename stepCount to something like retries? src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68338> no need for const on pass by value src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68336> s/string &/string& / src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68337> s/string &/string& / can this be const? src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68340> pass by value, no need for const& src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68343> no need for reference src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68341> no need for reference src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68342> no need for reference src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68344> s/Flags &/Flags& / src/slave/containerizer/pluggable_containerizer.hpp <https://reviews.apache.org/r/17567/#comment68345> kill newline src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68346> alphabetize src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68347> : and initializers on same line src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68348> {} at end of initializers src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68435> these shouldn't be fatal src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68436> these shouldn't be fatal src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68349> isn't this setting environment variables in the slave's environment!? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68351> isn't this modifying the slave's environment? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68356> is this promise needed? could you not do: return read().onAny(/* check result */); src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68357> shouldn't the file descriptor always be closed, i.e., .onAny()? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68406> as above, wouldn't it be better to return the continuation rather than passing around this promise? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68358> onAny? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68360> s/Resource */Resource* / src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68361> onAny? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68364> likewise, can you not do away with this promise and return from the await.then? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68362> onAny? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68365> onAny? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68403> can you please move all of the continuations to immediately after each method src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68410> can you re-use the code from Posix{Cpu,Memory}Isolator if it was pulled out? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68437> add a comment that resources aren't known after recovery unless update() is called src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68412> this seems a little draconian, why do you need to terminate the container, couldn't you just fail the statistics? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68438> why do you need to pass the future to commandSupported? why not check if the future is ready, then pass in the ResultFutures. src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68382> what about simplifying it a little if (!contains) { LOG(); } sandboxes.erase(); src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68383> ditto src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68447> if this was retries then this branch occurs which retries == 0, and it's unsigned. src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68449> shouldn't be a CHECK src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68450> Wouldn't this be cleaner as retryPeriod and numRetries ? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68370> this has now been added stout/abort.hpp src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68369> can this be replaced by the new Subprocess abstraction? src/slave/containerizer/pluggable_containerizer.cpp <https://reviews.apache.org/r/17567/#comment68453> refactor to avoid if/else pid = fork(); if (pid == 0) { // in child } // continue in parent src/slave/flags.hpp <https://reviews.apache.org/r/17567/#comment68332> why not call this default_image or default_container_image src/slave/flags.hpp <https://reviews.apache.org/r/17567/#comment68331> 'fails' is a bad word ;-), what about "The default container image to use if not specified by a task." - Ian Downes On March 10, 2014, 4:10 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17567/ > ----------------------------------------------------------- > > (Updated March 10, 2014, 4:10 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, > Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-816 > https://issues.apache.org/jira/browse/MESOS-816 > > > Repository: mesos-git > > > Description > ------- > > This patch adds the so-called pluggable containerizer. This > containerizer delegates all containerizer calls directly to > an external, pluggable containerizer program (which can be > specified on start-up). Few calls have internal fall-back > implementations such as wait(), destroy() and usage(). > > The protocol for the interactions with the external program > is as follows: > > COMMAND (ADDITIONAL-PARAMETERS) < INPUT-PROTO > RESULT-PROTO > > launch (ContainerID, --mesos-executor, <path>) < TaskInfo > PluggableStatus > update (ContainerID) < ResourceArray > PluggableStatus > usage (ContainerID) > ResourceStatistics > wait (ContainerID) > PluggableTermination > destroy (ContainerID) > PluggableStatus > > When protocol buffers need to be provided, the Mesos side of > the pluggable containerizer implementation will serialize the > protos on stdin and vice-versa for reading protos on stdout as > drafted in the above scheme. > > > Diffs > ----- > > configure.ac 390f11b > include/mesos/mesos.proto 37f8a7f > src/Makefile.am 384b312 > src/examples/python/test-containerizer.in PRE-CREATION > src/examples/python/test_containerizer.py PRE-CREATION > src/slave/containerizer/containerizer.cpp d0a1023 > src/slave/containerizer/pluggable_containerizer.hpp PRE-CREATION > src/slave/containerizer/pluggable_containerizer.cpp PRE-CREATION > src/slave/flags.hpp c9a627b > src/tests/pluggable_containerizer_test.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17567/diff/ > > > Testing > ------- > > make check and functional testing. > > > Thanks, > > Till Toenshoff > >
