----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16147/#review31605 -----------------------------------------------------------
src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60189> Not used? src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60188> None of these headers appear to be used. src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60190> Not used? src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60184> A comment in the header that the Containerizer will spawn the process (and terminate and wait for it in the destructor) would be nice. src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60185> Complete sentences please. src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60186> Complete sentences please. src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60187> Formatting is off here. src/slave/container/containerizer.cpp <https://reviews.apache.org/r/16147/#comment60191> In the future, use Error("unimplemented") instead. ;) src/slave/container/mesos_containerizer.hpp <https://reviews.apache.org/r/16147/#comment60193> Use const please for arguments. src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60194> Let's not use the '_' prefix unless it's actually a continuation. src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60195> Let's call this 'prepare', and s/_fork/fork/ and others too. Let's reserve the '_' prefix for continuations of the same function rather than chained functions. src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60196> Oooh, I like the use of CommandInfo! Can we s/scripts/commands/ everywhere? I like the semantics of an isolator might provide extra commands that need to get run! src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60197> s/lamba/lambda/ src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60198> Maybe include the container ID that was unknown? src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60199> Maybe include the container ID that was unknown? src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60201> Is there something else we can print to know why or what we are skipping? src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60203> Is there any reason to not include the launcher's termination message too? Maybe you should just append all messages together? If nothing more some comments about why you made the decisions you made would be great. src/slave/container/mesos_containerizer.cpp <https://reviews.apache.org/r/16147/#comment60202> s/has been limited in/has reached it's limit for/ src/slave/main.cpp <https://reviews.apache.org/r/16147/#comment60181> Wrap please. src/slave/main.cpp <https://reviews.apache.org/r/16147/#comment60182> Wrap please. src/slave/monitor.hpp <https://reviews.apache.org/r/16147/#comment60168> Why hashmap to std::map? src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment60165> Not your bug, but we should be using 'path::join' instead please. src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment60169> If you use a hashmap as the original code did you could use 'contains'. src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment60166> We should then be using 'path::join' here too. src/slave/monitor.cpp <https://reviews.apache.org/r/16147/#comment60167> And then 'path::join' here too, and everywhere else. Given this, it would be much cleaner to have a helper which did everything for us: // Helper to create a namespaced statistic name given an ExecutorInfo // and the statistic. We namespace the statistics like a path. string name(const ExecutorInfo& info, const std::string& statistic) { return path::join( info.framework_id().value(), info.executor_id().value, statistic); } Then, rather than pulling out a "prefix" and appending the statistic name everywhere we could do: name(executorInfo, CPU_USAGE) For example, assuming we had pulled out an 'info' representing the ExecutorInfo: ::statistics->set( "monitor", name(info, CPUS_USER_TIME_SECS), statistics.cpus_user_time_secs(), time); Is there any reason why we need to be using 'monitor' as the context as well? src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60175> Any reason not to do this call up above where you deleted the code and then keep the info.mutable_resources()->MergeFrom(resources.get()); in the same place? src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60174> s/MergeFrom/CopyFrom/ src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60176> Please wrap. src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60177> If we aren't going to use 'future', let's not even put it in the signature (and kill the lambda::_1 below). src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60178> You're missing a ' after frameworkId. Also, do you want to print out the reason monitoring termination failed (i.e., termination.failure())? Also, where do we document that a failed or discarded termination future implies the executor has actually terminated? Do we need, or want, to force the slave to destroy the container? Let's be explicit about the semantics here. Even more generally, does executor termination == container destroyed? src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60179> Wrap please. src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60180> Can you do: .then(defer(lambda::bind(&Containerizer::recover, containerizer, state))); Or also: .then(lambda::bind(&Containerizer::recover, containerizer, state)); The defer version is preferred but not strictly necessary. src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60171> Option<string> user = slave->flags.switch_user ? info.user() : None(); In fact, you can just use the right hand side where ever you use 'user'. src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60170> This is not doing what you expect, this is returning a new Option that is "some" with value info.user(). src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60172> Kill the helper and just create and populate the map right here, it's less lines of code (the helper doesn't really help). src/slave/slave.cpp <https://reviews.apache.org/r/16147/#comment60173> Wrap please. - Benjamin Hindman On Dec. 21, 2013, 6:50 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16147/ > ----------------------------------------------------------- > > (Updated Dec. 21, 2013, 6:50 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas > Nielsen, samya, and Jason Dusek. > > > Repository: mesos-git > > > Description > ------- > > The proposed Containerizer interface is to replace the existing Isolator. > > One ContainerizerProcess has been written: > MesosContainerizerProcess - implements containerizeration internally using a > Launcher and one or more Isolators (following review) > > The intent is to also support a generic ExternalContainerizerProcess that can > delegate containerizeration by making external calls. Other Containerizers > could interface with specific external containerization techniques such as > Docker or LXC. > > > Diffs > ----- > > include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34 > src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1 > src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1 > src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a > src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af > src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf > src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905 > src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886 > src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735 > src/slave/container/containerizer.hpp PRE-CREATION > src/slave/container/containerizer.cpp PRE-CREATION > src/slave/container/mesos_containerizer.hpp PRE-CREATION > src/slave/container/mesos_containerizer.cpp PRE-CREATION > src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb > src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc > src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f > src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37 > src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578 > src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa > src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028 > src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b > src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 > src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf > src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c > src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff > src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43 > src/slave/status_update_manager.hpp > 06ea4659cdd24cb1b82f389f404566ba14a663fb > src/slave/status_update_manager.cpp > f7a0c40f1b1a8cfab828998146b8f446a0569251 > src/tests/test_containerizer.hpp PRE-CREATION > src/tests/test_containerizer.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/16147/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
