----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review12429 -----------------------------------------------------------
You could have tests for this stuff in isolation. src/messages/messages.proto <https://reviews.apache.org/r/7212/#comment26337> Move type closer to the enum, and make it '= 1' (it's the thing that differentiates the type of message you have). src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26338> Why not match the filename? s/STATUSUPDATES/STATUS_UPDATES/ src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26344> There needs to be a high-level comment here describing the semantics of receiving (from an executor, when is the executor ACKed?), recording (what about frameworks that don't want the penalty of recording?), and sending status updates (how long are they resent?). Likewise, describing a few of the data structures here such as what a "status update stream" is, how we "name" it, how long it sticks around, etc. src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26339> virtual src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26340> Use an Option (and default it to none) if you're going to optionally send a message based on executorPID == UPID(). src/slave/status_updates_manager.hpp <https://reviews.apache.org/r/7212/#comment26341> const & src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26364> If you used the slave's PID rather than self (line 243) then the slave could dispatch to statusUpdateAcknowledgement which would give the added benefit of pushing any errors in recording the status update acknowledgement back to the slave to take action. It's more work on the slave side, but it enables capturing those errors fairly cleanly. I'm happy to discuss this more in detail. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26361> Space. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26362> ? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26342> s/master=/master =/ src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26343> If the slave removed the executor from it's data structures, and the slave dispatches to the SUM, how is this case possible? This sounds like it should be more of a CHECK than a LOG. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26345> We might need to rethink the 'acknowledged' set, since it grows (without bound) with the number of tasks. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26346> Incomplete sentence. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26363> You could return the error through the future and let the slave commit suicide. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26347> How about having sendStatusUpdate return the timeout (since delay returns a Timer from which you can call Timer::timeout)? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26353> Is this just your guess about how this will work? Or is there code that actually uses this? If so, I'd love to see that review too please. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26365> const & src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26354> s/no associated/no associated/ src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26355> Yes, this should be an ERROR since two status updates should not be in flight at the same time. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26356> Can drop the '} else {' and unindent that block (that's why you have the 'return' on the previous line). src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26358> Just to be pedantic, you need to do this AFTER you write it to disk. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26359> As mentioned above (and below): stream->timeout = sendStatusUpdate(...); src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26360> return delay(...).timeout(); src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26352> This is TODO. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26357> The update can have a large 'data' chunk. Any reason not to just write the UUID so that we don't have to write the data chunk twice? src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26348> Same comment as above, this could be: stream->timeout = sendStatusUpdate(update); src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26349> Better yet, do a get which should return an Option. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26350> That works for me. src/slave/status_updates_manager.cpp <https://reviews.apache.org/r/7212/#comment26351> Again, Option seems best here. And delete after you remove from the data structure please. ;) - Benjamin Hindman 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 > >
