> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, lines 907-908 > > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line907> > > > > So maybe status should an Option, both in the CgroupInfo struct and > > passed back to the slave?
I thought about that. But the issue is that 'status' is a required field in the ExitedExecutorMessage protobuf. So, a non-existence of status should be converted to "-1" at some point before sending it to the master. One option is to make the field optional, but I guess that makes it backwards incompatible (or rather enforces a "master first then slaves" deploy). Let me know your thoughts? > On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1094 > > <https://reviews.apache.org/r/9408/diff/8/?file=260460#file260460line1094> > > > > Is this a bug fix? No. It just has been moved up from shutdownExecutorTimeout(). > On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1146 > > <https://reviews.apache.org/r/9408/diff/8/?file=260460#file260460line1146> > > > > 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! More importantly, I think its wrong to remove/gc an executor before it even exited! Because, it could give the wrong impression (for e.g webui) on whether an executor is still running or it could pull an executor's working directory from underneath it while its still running. > On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 902 > > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line902> > > > > 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. Well, my concern is that there is nothing stopping someone from calling _killExecutor() with a different 'cgroup' name than the one that the 'info' corresponds to. > On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 896 > > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line896> > > > > 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). turns out i just need to copy the framework and executor ids when calling unregisterinfo. fixed. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9408/#review16870 ----------------------------------------------------------- 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 > >
