> On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/exec/exec.cpp, line 348 > > <https://reviews.apache.org/r/8762/diff/6/?file=260150#file260150line348> > > > > Should 'slaveId' be an option? I don't really like the .IsInitialized > > semantics.
fair enough. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/exec/exec.cpp, line 398 > > <https://reviews.apache.org/r/8762/diff/6/?file=260150#file260150line398> > > > > Why this change? reverted. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/exec/exec.cpp, line 432 > > <https://reviews.apache.org/r/8762/diff/6/?file=260150#file260150line432> > > > > At this point in the review I still don't exactly know how/why you're > > saving all the tasks. I see you do a duplicate check but that seems a bit > > crazy to save the tasks. Also, sometimes you refer to the tasks as > > "unacknowledged" and sometimes you refer to them as "un-updated" ... some > > more comments here are probably necessary. added a comment. note that this map is going to be very small, because we remove a task from this map as soon as we get an ACK for it. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/exec/exec.cpp, line 556 > > <https://reviews.apache.org/r/8762/diff/6/?file=260150#file260150line556> > > > > Doing numify<bool> seems a little weird here. Why not just: > > > > checkpoint = string(value) == "1"; > > > > Or something like it? Likewise, if you replace all of the getenv calls > > here with their modern os::getenv versions then you can get a string back > > instead of a char*. sure. also, error checking is free with os::getenv(), which is great. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 920 > > <https://reviews.apache.org/r/8762/diff/6/?file=260156#file260156line920> > > > > Not your bug, but s/tag/uuid/g. Also, I'm much more inclined to always > > store UUIDs as the UUID type in code and only use toString to pretty print > > them. Then people don't have to think about what the representation of this > > UUID might be given that a string could either be the pretty-printed > > version or just the bytes. This also isn't really your bug, but we should > > fix this. Note that when we use the UUID to construct the cgroup name or > > directories then it's totally fine to use the pretty-printed version, let's > > just do it there not here. sgtm. fixed. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 401 > > <https://reviews.apache.org/r/8762/diff/6/?file=260166#file260166line401> > > > > I think we need a more descriptive name for 'active' (something more > > along the lines of shutdown like 'halt' or 'halted' or 'halting'. i like halting. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1122 > > <https://reviews.apache.org/r/8762/diff/6/?file=260166#file260166line1122> > > > > Maybe we should do this first? Before we handle status updates or > > relaunch any tasks? Sure. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1152 > > <https://reviews.apache.org/r/8762/diff/6/?file=260166#file260166line1152> > > > > What "event" does this deliver? How is it used? Some comments here are > > necessary. Finally, who owns the promise? Is this is a memory leak? Added a comment. And yes, this is a memory leak! Thank you. Changed 'recovered' from future to promise to fix this. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1501 > > <https://reviews.apache.org/r/8762/diff/6/?file=260166#file260166line1501> > > > > s/:/,/ n/a as i got rid of this logic altogether. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1726 > > <https://reviews.apache.org/r/8762/diff/6/?file=260166#file260166line1726> > > > > So, we have the isolation module recover this executor even if we're > > just killing it? Probably needs some more comments about this interplay. Isolation module recovery basically entails getting enough information to interact with an executor (change resources, monitor usage, killing executor) etc. Added a comment. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/status_update_manager.cpp, line 71 > > <https://reviews.apache.org/r/8762/diff/6/?file=260169#file260169line71> > > > > Are there other kinds of directories a recover function would need? No. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/lxc_isolation_module.cpp, line 116 > > <https://reviews.apache.org/r/8762/diff/6/?file=260162#file260162line116> > > > > Just elide the paramater name entirely. I thought the convention of unused params is to use "_" ? I see this in another places too. > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/slave/slave.hpp, line 222 > > <https://reviews.apache.org/r/8762/diff/6/?file=260165#file260165line222> > > > > Comment or add a TODO on why const&. not needed anymore after your refactor of future.then(). > On Feb. 23, 2013, 11:49 p.m., Benjamin Hindman wrote: > > src/tests/slave_recovery_tests.cpp, line 185 > > <https://reviews.apache.org/r/8762/diff/6/?file=260170#file260170line185> > > > > Is there a way to just pass in the type T and then instantiate that > > directly? I think Thomas might have done something like this in allocator > > tests. If we can do without using the factory here that will save someone > > time when we eliminate these factories for factory functions. Done. Its too bad that to access templatized base class members you need to use "this->" in C++. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8762/#review16990 ----------------------------------------------------------- On Feb. 20, 2013, 2:10 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8762/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2013, 2:10 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Properly recovers and reconnects with executors. > > Also, recovers isolation module. > > This is pretty much the whole of recovery! (I'm going to send out another > tiny review for properly doing incompatible upgrades) > > > Diffs > ----- > > src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24 > src/launcher/launcher.hpp ead58c0f60b6bff532f241a224b547b25e292175 > src/launcher/launcher.cpp f2d68c5fd1201b75c5a8f96599d7fb36516687fb > src/launcher/main.cpp e90df85ae433431defdd251490c74a482a58f743 > src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f > src/slave/cgroups_isolation_module.hpp > 669efa14ba2603764aa68ae19a44e79dbfdec192 > src/slave/cgroups_isolation_module.cpp > 14f549edaf1b37a6bca8f75309864333ae775e7c > src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c > src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000 > src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd > src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3 > src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4 > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c > src/slave/process_based_isolation_module.hpp > f1817192582e3646f8dcf17934ba7998829e8fd6 > src/slave/process_based_isolation_module.cpp > 12a579cba56cd3dac384bc7919b0d5537b0e429d > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd > src/slave/state.cpp 2688567719c5b12f1d8a56fa6193c11d4077685b > src/slave/status_update_manager.hpp PRE-CREATION > src/slave/status_update_manager.cpp PRE-CREATION > src/tests/slave_recovery_tests.cpp PRE-CREATION > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 > > Diff: https://reviews.apache.org/r/8762/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
