----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9408/#review16974 -----------------------------------------------------------
Sorry for not spotting this stuff earlier! src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35970> Can you use a different continuation here? I see this is why you added the cgroup name and info, but really the continuation for this one is really simple (just LOG(INFO)). This will also simplify the logic inside _killExecutor. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35959> I'm not sure I agree with benh here (or he didn't see this bit in terms of asking why the lock fd was an option). Now you have a sentinel value (-1) to represent the concept of 'none'. These are the exact semantics abstracted away by the option abstraction. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35967> Just pass info, since it includes info->name(). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35965> Now that you're passing info, can't you remove the cgroup string argument? It now seems redundant and allows a mismatch between info->name and cgroup. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35962> since you're no longer making a copy, why introduce a variable? Seems better to just call the option 'info' and use get() here and below? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35966> This check would be eliminated by removing the cgroup argument. src/slave/process_based_isolation_module.cpp <https://reviews.apache.org/r/9408/#comment35958> kill the comment now? src/slave/slave.cpp <https://reviews.apache.org/r/9408/#comment35960> s/till/until Can you satisfy the TODO by calling: terminate(isolationModule, false) This will not inject the terminate message in front of the queue, which seems like the right thing to do, right? src/slave/slave.cpp <https://reviews.apache.org/r/9408/#comment35961> Can you add a TODO where the completedFrameworks is declared in the slave header for me to used the Owned abstraction? (Owned was introduced in the monitoring reviews) - Ben Mahler On Feb. 22, 2013, 2:49 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9408/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2013, 2:49 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 > a2eba6f96f5d8a4b1257571aa29e37c5682aab8d > 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 > >
