> On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.hpp, line 53 > > <https://reviews.apache.org/r/7212/diff/1/?file=159110#file159110line53> > > > > so.. does each stream have a file where it's checkpointing status > > updates? > > > > Perhaps s/path/filepath is more appropriate?
Yes. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.hpp, line 111 > > <https://reviews.apache.org/r/7212/diff/1/?file=159110#file159110line111> > > > > Might be better to use Option<int> for the fd, rather than using -1 as > > a sentinel value. agreed. done > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.hpp, line 151 > > <https://reviews.apache.org/r/7212/diff/1/?file=159110#file159110line151> > > > > where is run used? This is used by the slave, during recovery. That code hasn't been sent out for review yet. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 66 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line66> > > > > What is path indicating here? Path is where the status update is checkpointed. It is doc'ed in the header. Let me know if it isn't clear. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 80 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line80> > > > > use path.empty()? done > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 81 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line81> > > > > capitalize Its the variable name. quoted it for clarity. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 154 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line154> > > > > any reason for checking this, the input is a hashmap and so by > > definition the keys are unique? killed > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 215 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line215> > > > > I think inlining these is more intuitive, and they get evaluated once > > anyway. this is an inlined function! also, this will be used in slave.cpp too. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 226 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line226> > > > > 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? these are acked by the scheduler driver. agree that this could be a scalability problem, but i think these kinda optimizations are way down the road. > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 234 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line234> > > > > period, also maybe just have the full comment at the signature rather > > than down here? ditto on the others done > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 318 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line318> > > > > why not do a find to avoid doing the double lookup? done > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 332 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line332> > > > > use CHECK_NOTNULL and inline it in the assignment: > > > > streams[id] = CHECK_NOTNULL(stream); > > > > actually.. benh might not like inlining it done. i like it :) > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 344 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line344> > > > > sanity check for contains (use find to avoid double lookups) done > On Sept. 28, 2012, 1:14 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 195 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line195> > > > > seems indicative of an ERROR done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review12010 ----------------------------------------------------------- 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 > >
