> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/container/containerizer.cpp, line 19 > > <https://reviews.apache.org/r/16147/diff/4/?file=401861#file401861line19> > > > > Not used?
Used for ContainerizerProcess::create implementation which will appear in next update. > On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/monitor.hpp, line 135 > > <https://reviews.apache.org/r/16147/diff/4/?file=401868#file401868line135> > > > > Why hashmap to std::map? Relic from when I hadn't defined hash_value(ContainerID) I think. > On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, line 117 > > <https://reviews.apache.org/r/16147/diff/4/?file=401869#file401869line117> > > > > We should then be using 'path::join' here too. I'll defer these sort of refactors to [~bmahler]'s monitor changes. > On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, line 118 > > <https://reviews.apache.org/r/16147/diff/4/?file=401869#file401869line118> > > > > 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? I'll defer these sort of refactors to [~bmahler]'s monitor changes. > On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 2161 > > <https://reviews.apache.org/r/16147/diff/4/?file=401874#file401874line2161> > > > > 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? I've added a comments to make it explicit that the intended semantics were the containerized context is automatically destroyed when the executor terminates. However, I'm not 100% sure this is the preferred semantics, particularly for other containerizers? Comments? > On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 2699 > > <https://reviews.apache.org/r/16147/diff/4/?file=401874#file401874line2699> > > > > 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. Neither of these are recognized and compile. See this review for a minimal example https://reviews.apache.org/r/13782/ - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16147/#review31605 ----------------------------------------------------------- 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 > >
