> On Aug. 13, 2013, 7:28 p.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, lines 204-205 > > <https://reviews.apache.org/r/13520/diff/1/?file=340298#file340298line204> > > > > 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)
sgtm. thanks. fixed. > On Aug. 13, 2013, 7:28 p.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, lines 203-206 > > <https://reviews.apache.org/r/13520/diff/1/?file=340298#file340298line203> > > > > 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. I like the @return format that we used in cgroups.cpp, cgroups_isolator.hpp, fs.hpp and elsewhere in this file. I think its OK if Doxygen cannot understand it, I still think this format is more clear. I reformatted it a bit to be consistent with @return's elsewhere in the code base. > On Aug. 13, 2013, 7:28 p.m., Ben Mahler wrote: > > src/slave/status_update_manager.cpp, lines 325-329 > > <https://reviews.apache.org/r/13520/diff/1/?file=340299#file340299line325> > > > > 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? sg. done > On Aug. 13, 2013, 7:28 p.m., Ben Mahler wrote: > > src/tests/status_update_manager_tests.cpp, line 684 > > <https://reviews.apache.org/r/13520/diff/1/?file=340300#file340300line684> > > > > In a previous review you used CopyFrom instead of the assignment > > operator, let's try to be consistent in the tests. We do the assignment everywhere, except slave recovery tests. I will fix that instead. > On Aug. 13, 2013, 7:28 p.m., Ben Mahler wrote: > > src/tests/status_update_manager_tests.cpp, line 745 > > <https://reviews.apache.org/r/13520/diff/1/?file=340300#file340300line745> > > > > Can you mention that you're advancing the clock to trigger the status > > update retry? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13520/#review25088 ----------------------------------------------------------- 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 > >
