> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote: > > src/slave/slave.hpp, line 139 > > <https://reviews.apache.org/r/7655/diff/11/?file=261368#file261368line139> > > > > Document the purpose of the optional executor argument, maybe use > > Option (with None() default) + CHECK_NOTNULL when the option is some?
Aah. I don't like options for pointers. But, added a comment. > On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 76 > > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line76> > > > > why does it need to be a pointer at all? The idea was to make it easy to inject a mock status update manager for testing (like we do isolation module). But, I never got around it :/ I can create this on a stack, if this bothers you. > On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 610 > > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line610> > > > > I don't buy this comment, statusUpdate is functionally doing more than > > forwardUpdate. > > > > It is modifying the executor state, if found. Talking to the isolation > > module as well. > > > > Also, if statusUpdate forwarded for unknown frameworks we could call it > > here right? Sounds like we should take care of that TODO and eliminate the > > need for having _both_ statusUpdate and forwardUpdate, wouldn't that be > > much cleaner? OK. After our discussions offline, I think the right thing to do here is to make statusUpdate() more permissive (i.e., forwarding updates irrespective of unknown frameworks/executors). We can revisit this strategy when there is persistent state at the master. I am keeping forwardUpdate() instead of folding it statusUpdate() to make it easy to revert the logic in the future, when the master has persistence. > On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 686 > > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line686> > > > > s/shut down/shutdown to be consistent I think BenH doesn't like 'shutdown' in sentences. The log message above is talking about 'shutdown' framework message, which I think is ok. > On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 791 > > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line791> > > > > Use "Failed" style message. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7655/#review17204 ----------------------------------------------------------- On Feb. 23, 2013, 8:11 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7655/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2013, 8:11 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Integrated SUM into slave. > > > This addresses bug MESOS-110. > https://issues.apache.org/jira/browse/MESOS-110 > > > Diffs > ----- > > include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801 > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 > src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b > src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd > src/slave/status_update_manager.hpp PRE-CREATION > src/tests/master_tests.cpp e140f409bf6e651cc365b5fb8d064ceae1cd8ed8 > src/tests/status_update_manager_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/7655/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
