----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64095/#review192544 -----------------------------------------------------------
src/status_update_manager/status_update_manager_process.hpp Lines 654 (patched) <https://reviews.apache.org/r/64095/#comment270763> Actually, after looking at this again I was thinking: perhaps this comment should be changed to tell readers that we are doing two things here: both building our in-memory streams and the state object which we return to the caller. WDYT? src/status_update_manager/status_update_manager_process.hpp Lines 666 (patched) <https://reviews.apache.org/r/64095/#comment270759> Could we use a `switch` here instead of `if`? src/status_update_manager/status_update_manager_process.hpp Lines 678 (patched) <https://reviews.apache.org/r/64095/#comment270761> s/.get()./->/ src/status_update_manager/status_update_manager_process.hpp Lines 692 (patched) <https://reviews.apache.org/r/64095/#comment270769> As written, this is a bit opaque. I had to look up why we're seeking from the current position with an offset of zero. I now see that we're doing this in an attempt to return the current file position, ensuring that the current position is valid. Could you clarify this? I could imagine doing it by renaming this variable from `lseek` to `currentPosition`, or putting a note in the comment above. src/status_update_manager/status_update_manager_process.hpp Lines 709-710 (patched) <https://reviews.apache.org/r/64095/#comment270771> Fits on one line. src/status_update_manager/status_update_manager_process.hpp Lines 717 (patched) <https://reviews.apache.org/r/64095/#comment270775> Could you get rid of this `else`? src/status_update_manager/status_update_manager_process.hpp Lines 725 (patched) <https://reviews.apache.org/r/64095/#comment270776> We could probably use a `std::pair` here instead. And maybe we can use `std::make_pair`, as long as it doesn't have issues with type deduction? src/status_update_manager/status_update_manager_process.hpp Lines 734 (patched) <https://reviews.apache.org/r/64095/#comment270783> s/duplicate/duplicate or has already been acknowledged/ src/status_update_manager/status_update_manager_process.hpp Lines 747 (patched) <https://reviews.apache.org/r/64095/#comment270780> s/uuid/status_uuid/ src/status_update_manager/status_update_manager_process.hpp Lines 754 (patched) <https://reviews.apache.org/r/64095/#comment270782> Could you remove the exclamation point? src/status_update_manager/status_update_manager_process.hpp Lines 780-783 (patched) <https://reviews.apache.org/r/64095/#comment270787> I know that in this context, using just `uuid` is not as ambiguous, but given the presence of multiple UUIDs in general I think it's better to be explicit. Could we use `status_uuid` for the function argument? src/status_update_manager/status_update_manager_process.hpp Lines 790-791 (patched) <https://reviews.apache.org/r/64095/#comment270790> Since we print the status UUID in the stream operator for the update type, we should be able to eliminate the explicit UUID in the log line here and not lose any information. src/status_update_manager/status_update_manager_process.hpp Lines 847-849 (patched) <https://reviews.apache.org/r/64095/#comment270810> I think it's more common for us to keep the empty function body on the same line: ``` : terminated(false), streamId(_streamId), path(_path), fd(_fd) {} ``` Here and elsewhere. src/status_update_manager/status_update_manager_process.hpp Lines 855 (patched) <https://reviews.apache.org/r/64095/#comment270812> s/its/it's/ src/status_update_manager/status_update_manager_process.hpp Lines 873 (patched) <https://reviews.apache.org/r/64095/#comment270813> Ditto - use a `switch` here. src/status_update_manager/status_update_manager_process.hpp Lines 885-886 (patched) <https://reviews.apache.org/r/64095/#comment270815> Hmm this error message seems strange in the ACK case? src/status_update_manager/status_update_manager_process.hpp Lines 905 (patched) <https://reviews.apache.org/r/64095/#comment270816> Ditto - use a `switch` here. src/status_update_manager/status_update_manager_process.hpp Lines 907-908 (patched) <https://reviews.apache.org/r/64095/#comment270823> Let's pass framework ID into the creation method to eliminate the NONE case here. src/status_update_manager/status_update_manager_process.hpp Lines 912 (patched) <https://reviews.apache.org/r/64095/#comment270819> I think we can remove this comment. src/status_update_manager/status_update_manager_process.hpp Lines 923 (patched) <https://reviews.apache.org/r/64095/#comment270824> Can remove this comment. - Greg Mann On Dec. 1, 2017, 1:42 a.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64095/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2017, 1:42 a.m.) > > > Review request for mesos and Greg Mann. > > > Bugs: MESOS-8197 > https://issues.apache.org/jira/browse/MESOS-8197 > > > Repository: mesos > > > Description > ------- > > This actor handles the checkpointing, recovery, and retry of status > updates. > > It will initially be used by the offer operations status update > manager, but it was designed and implemented so that it can replace > the current implementation of the task status update manager. > > > Diffs > ----- > > src/status_update_manager/status_update_manager_process.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/64095/diff/3/ > > > Testing > ------- > > Added new tests at the end of the chain. They passed 5000 times on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
