----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7655/#review17204 -----------------------------------------------------------
The higher level message expressed in my comments is whether we can push forwardUpdate into statusUpdate and eliminate the need for both calls. It looks like this is possible if you take care of the TODO for forwarding unknown framework updates, right? include/mesos/mesos.proto <https://reviews.apache.org/r/7655/#comment36487> I see you're following the documentation style of mesos.proto but I think at some point we should move to document the fields inline over including them in the large paragraph above the proto message. src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment36492> Can you group the declaration of the status update methods so they are all together? i.e. why are the status update ack calls declared separately above? src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment36491> Document the purpose of the optional executor argument, maybe use Option (with None() default) + CHECK_NOTNULL when the option is some? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36493> why does it need to be a pointer at all? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36497> 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? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36498> ditto above. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36499> s/shut down/shutdown to be consistent src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36500> Use "Failed" style message. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36501> Can you include the same information as above? Task id and framework id specifically. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36496> s/,// s/at/in/ src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment36495> I don't see why we forward when the executor is unknown but _not_ when the framework is unknown? I see you have a TODO above for this very question. What exactly would be the harm in forwarding it to the master? - Ben Mahler 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 > >
