----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9408/#review16870 -----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp <https://reviews.apache.org/r/9408/#comment35801> Why is this an option? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35803> 'None()' (after trunk rebase). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35802> PLOG(FATAL) src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35807> This doesn't feel like an error since you call his with None above in the code! This should probably just be a return (no logging, the logging line above is sufficient). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35804> I do not understand why you need a copy. And we are generally averse to making copies of things we have pointers to (as it usually implies they have some non-copyable state, otherwise we could avoid pointers all together). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35808> This one feels a bit gratuitous. Assertions should point out things you might be concerned about getting wrong in the code. The one above says to me: "I only expect to get to _killExecutor with a non-none CgroupInfo if the executor for that cgroup was killed". That tells me that there is some complicated semantics around when/how _killExecutor might get invoked and I should be weary of that if I ever make changes in here. However, this assertion makes me think that there are some complicated semantics around how the cgroup name might change depending on how _killExecutor gets called ... which I don't think is correct thus leaving the wrong message. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35806> So maybe status should an Option, both in the CgroupInfo struct and passed back to the slave? src/slave/slave.cpp <https://reviews.apache.org/r/9408/#comment35810> Is this a bug fix? src/slave/slave.cpp <https://reviews.apache.org/r/9408/#comment35811> I guess the guarantee that we didn't do this twice was that we couldn't lookup the executor id twice (since we had destroyed the executor on the next line). I like the way you've done it now MUCH better! src/tests/balloon_framework_test.sh <https://reviews.apache.org/r/9408/#comment35809> How about: "This function is not pure, but depends on state from the environment (e.g., ${SLAVE_PID}) because we do not know all possible values when we register this function with 'atexit'". Related, the last thing I'd like to do here is "declare" MASTER_PID and SLAVE_PID above the cleanup function (above the comment, below the unmount stuff). This should be sufficient: MASTER_PID= SLAVE_PID= As a way of signifying that these are global environment variables that we expect (rather than referencing them in cleanup before they are declared). Sound good? - Benjamin Hindman On Feb. 21, 2013, 6:40 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9408/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2013, 6:40 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > See summary > > > Diffs > ----- > > src/slave/cgroups_isolation_module.hpp > 669efa14ba2603764aa68ae19a44e79dbfdec192 > src/slave/cgroups_isolation_module.cpp > 14f549edaf1b37a6bca8f75309864333ae775e7c > src/slave/process_based_isolation_module.cpp > 12a579cba56cd3dac384bc7919b0d5537b0e429d > src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd > src/tests/balloon_framework_test.sh > 93a733f64cfde08349b7781eb3d5e13594c74498 > > Diff: https://reviews.apache.org/r/9408/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
