----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review12463 -----------------------------------------------------------
src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26471> 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. src/messages/messages.proto <https://reviews.apache.org/r/7212/#comment26478> 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. src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26497> Add these includes: #include <ostream> #include <queue> #include <string> #include <process/pid.hpp> src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26482> Enqueued for what? src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26485> Alternative phrasing: After checkpointing, an ACK is immediately sent to the executor. src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26481> s/re-tried/retried s/till/until s/received/received from the master/ src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26487> s/to master/to the master src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26488> s/ack/ACK src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26491> s/>r/> r/ src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26495> I think you can do s/acknowledgement/ACK s/sendStatusUpdateAcknowledgement/sendStatusUpdateAck given that the rest of the code / comments talk about 'ACK's. src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26508> s/rtype/recordType here and below. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26498> s/to master/to the master src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26499> s/slave/this slave right? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26500> s/executor/the executor src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26501> Is this comment accurate? It said to ignore it, but then it creates a stream and opens it? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26504> Was there a reason for the space? " ." src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26505> It seems like you'd want to pop() here, no? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26506> Kill newline. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26507> Can you document why you sometimes expect !pid.isSome()? That is, why is pid an Option? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26509> Extra indentation here. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26510> Seems like we should add a bool function to Timeout for this case. Something like: timeout.expired(); timeout.timedOut(); What do you think? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26511> Extra semicolon. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26512> Rather than return null here, you could just return the Option directly, and then there's no need for this function? - Ben Mahler 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 > >
