----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review12010 -----------------------------------------------------------
src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment25630> so.. does each stream have a file where it's checkpointing status updates? Perhaps s/path/filepath is more appropriate? src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment25624> Might be better to use Option<int> for the fd, rather than using -1 as a sentinel value. src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment25623> I had written all these comments about fsync() and O_SYNC, then I see you've got it down here! ;) src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment25625> where is run used? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25629> What is path indicating here? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25626> use path.empty()? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25627> capitalize src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25628> The comment you wrote suggests ERROR over WARNING, no? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25621> any reason for checking this, the input is a hashmap and so by definition the keys are unique? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25620> seems indicative of an ERROR src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25619> Many of these comments are missing the trailing period. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25633> period src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25631> I think inlining these is more intuitive, and they get evaluated once anyway. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25632> who is ACKing these, the master? seems like a lot of communication for all the slaves to be sending every single status update in serial? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25634> period, also maybe just have the full comment at the signature rather than down here? ditto on the others src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25614> why not do a find to avoid doing the double lookup? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25616> use CHECK_NOTNULL and inline it in the assignment: streams[id] = CHECK_NOTNULL(stream); actually.. benh might not like inlining it src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment25617> sanity check for contains (use find to avoid double lookups) - Ben Mahler On Sept. 21, 2012, 6:20 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2012, 6:20 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > This is Status Update Manager's API and implementation. > > Haven't included the integration (into slave) and tests yet. > > Rebased off latest trunk > > > Diffs > ----- > > src/Makefile.am df8696920fd48968907270decbef3dda0803f80a > src/messages/messages.proto 2a0321cb3d9e3f6499e2108a3b21516a3bd18d2f > src/slave/status_updates_manager.hpp PRE-CREATION > src/slave/status_updates_manager.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/7212/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Vinod Kone > >
