> On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 215 > > <https://reviews.apache.org/r/7212/diff/1/?file=159111#file159111line215> > > > > By inlining I meant not storing the bools and using the expressions in > > the if conditions directly. > > > > if (protobuf::isTerminalState(update.status().state())) { > > if (!stream->pending.empty()) { > > > > I think it makes it more readable. Or at the very least: > > s/empty/emptyStream and s/terminal/terminalState but I would prefer the > > first option.
got rid of terminal and s/empty/emptyStream. i wanted 'empty' in a variable, because its used twice. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/messages/messages.proto, line 61 > > <https://reviews.apache.org/r/7212/diff/2/?file=176985#file176985line61> > > > > s/Consequently, i/NOTE: I > > > > I think this belongs above the Type enum itself to describe each type > > in the enum. That way they don't really need to be "NOTE:" comments either. I think it reads better at top. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.hpp, line 50 > > <https://reviews.apache.org/r/7212/diff/2/?file=176986#file176986line50> > > > > Enqueued for what? Re-worded a lot of the comments, with the refactor. So, I'm going to drop the below issues. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.hpp, line 94 > > <https://reviews.apache.org/r/7212/diff/2/?file=176986#file176986line94> > > > > I think you can do > > > > s/acknowledgement/ACK > > s/sendStatusUpdateAcknowledgement/sendStatusUpdateAck > > > > given that the rest of the code / comments talk about 'ACK's. All the comments now say ACK. I still like the functions to have the Acknowledged prefix. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 86 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line86> > > > > Is this comment accurate? It said to ignore it, but then it creates a > > stream and opens it? No longer relevant > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 113 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line113> > > > > Was there a reason for the space? " ." nope, fixed > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 190 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line190> > > > > It seems like you'd want to pop() here, no? pop what? > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 253 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line253> > > > > Can you document why you sometimes expect !pid.isSome()? That is, why > > is pid an Option? Sometimes slave generates TASK_LOST messages, precisely when it doesnt know about the executor. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 290 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line290> > > > > Extra indentation here. good eye! > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 292 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line292> > > > > Seems like we should add a bool function to Timeout for this case. > > Something like: > > > > timeout.expired(); > > timeout.timedOut(); > > > > What do you think? > > agreed, done. > On Oct. 16, 2012, 1:16 a.m., Ben Mahler wrote: > > src/slave/status_updates_manager.cpp, line 322 > > <https://reviews.apache.org/r/7212/diff/2/?file=176987#file176987line322> > > > > Rather than return null here, you could just return the Option > > directly, and then there's no need for this function? This is a convenience function to avoid that boilerpate needed for checking the option and getting the value out of it. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review12463 ----------------------------------------------------------- On Oct. 15, 2012, 11:46 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2012, 11:46 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Implementation of Status Updates Manager. > > Features: > --> Checkpoints status updates and acks. > --> Reliably sends stats updates > > > Diffs > ----- > > src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac > src/messages/messages.proto 4e0538fe929f9091e5cdd4a1bb017d836df52a3e > 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 > >
