----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13520/#review25088 -----------------------------------------------------------
The main thing here was that I'd like to avoid having Result<Nothing> in the code as it exposes two states of nothingness. FWIW, this will be the first instance of this in the code: ➜ mesos git:(master) ✗ grep -R 'Result<Nothing>' src ➜ mesos git:(master) ✗ grep -R 'Result<Nothing>' . ➜ mesos git:(master) ✗ src/slave/status_update_manager.hpp <https://reviews.apache.org/r/13520/#comment49276> My understanding is that @return was used in some places in the code in anticipation of using a documentation generation tool like Doxygen. AFAICT Doxygen et. al. do not support multi-line @return statements in this format. Can you comment this without using @return? Ditto below. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/13520/#comment49278> Result of Nothing vs. None Result seems like a subtle distinction to make. On that note, I don't think Result<Nothing> makes sense, since Result already has a state for nothing-ness. How about: // Returns true iff this update was handled, false if already handled (duplicate). // Returns an error if ... Try<bool> update(const StatusUpdate& update) src/slave/status_update_manager.hpp <https://reviews.apache.org/r/13520/#comment49281> Ditto on the @return. What about having true / false indicate whether this was processed instead of stream termination state? Then to ask about stream termination, one could call a stream.terminated() function instead of looking at the bool. This seems more explicit and intuitive to me, what do you think? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/13520/#comment49282> Here you could use the bool suggestion above to ask whether the update was processed. This feels a little more intuitive to me, since you want to know whether the result was "processed", which has a boolean nature to it. Thoughts? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/13520/#comment49283> In a previous review you used CopyFrom instead of the assignment operator, let's try to be consistent in the tests. src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/13520/#comment49285> Can you mention that you're advancing the clock to trigger the status update retry? - Ben Mahler On Aug. 13, 2013, 12:40 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13520/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2013, 12:40 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-640 > https://issues.apache.org/jira/browse/MESOS-640 > > > Repository: mesos-git > > > Description > ------- > > Refactored the StatusUpdateStream API a bit to make it easy to distinguish > duplicates from errors. > > > Diffs > ----- > > src/slave/status_update_manager.hpp > da927606dea63b62d99baac529a3fe371f2767ba > src/slave/status_update_manager.cpp > ffd47369e49f711f984787cd3710f7b4dd5b29ce > src/tests/status_update_manager_tests.cpp > 6473478a283d19f991143710bcf551e298bd6916 > > Diff: https://reviews.apache.org/r/13520/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
