----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10142/#review19115 -----------------------------------------------------------
1. All of the CHECK statements need to output the state. This could be obviated by just using switches / ifs everywhere. 2. I like the clarity of the switch statements, but they do have the downside that you sometimes use if statements, and sometimes use switch statements. I'm guessing this was to avoid nesting. Just a note, I'm good with the switches. src/slave/slave.hpp <https://reviews.apache.org/r/10142/#comment39670> Use either present or past tense in all of these? I prefer present tense: // Executor is launched, but not (re-)registered yet. ... src/slave/slave.hpp <https://reviews.apache.org/r/10142/#comment39671> If present tense above, then present tense here as well. src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39672> Why? src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39673> Signal to whom? src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39674> newline? src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39675> Can you either change this to an else if RUNNING or do a CHECK(RUNNING) on the framework state? src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39676> newline? src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39677> Ditto for the executor state here. Either if or CHECK for RUNNING, or all the expected states. src/slave/slave.cpp <https://reviews.apache.org/r/10142/#comment39678> Do you want to CHECK the expected states? - Ben Mahler On April 12, 2013, 9:24 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10142/ > ----------------------------------------------------------- > > (Updated April 12, 2013, 9:24 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > This is based off https://reviews.apache.org/r/10112. > > Also fixed TODOs and other misc stuff from the above review. > > > Diffs > ----- > > src/slave/isolator.hpp d702041784f5db159efd7da4d916405e86d99741 > src/slave/slave.hpp 2529bf500a3265b10ad4cddde10c2d62a6cdb4a0 > src/slave/slave.cpp 325231458a6883019436e7cc5a37f85f0f5735fa > src/slave/status_update_manager.hpp > e6ca40c5c05c0952cf76fb1db7eff2e4270c0d24 > src/slave/status_update_manager.cpp > 044d245f370ef23ddc67fadbf7f8fe9d75dd662a > src/tests/isolator.hpp f885ccb44e809383e658f45d9a03eda174cf2d72 > src/tests/slave_recovery_tests.cpp d0ff9b73e06e89a5409f038be2766333e0a0689e > > Diff: https://reviews.apache.org/r/10142/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
