----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64095/#review192409 -----------------------------------------------------------
Result of the review session with Graag. src/status_update_manager/status_update_manager_process.hpp Lines 55-56 (patched) <https://reviews.apache.org/r/64095/#comment270524> Let's try making this a nested class. Maybe we can name it `StatusUpdateStream` and get rid of the typedef. src/status_update_manager/status_update_manager_process.hpp Lines 69 (patched) <https://reviews.apache.org/r/64095/#comment270525> Reference here a JIRA for the migration of the `TaskStatusUpdateManager`. Say whether the manager will be paused or not on creation. src/status_update_manager/status_update_manager_process.hpp Lines 73 (patched) <https://reviews.apache.org/r/64095/#comment270526> Check what we do in our codebase regarding indentation of template parameters. src/status_update_manager/status_update_manager_process.hpp Lines 76 (patched) <https://reviews.apache.org/r/64095/#comment270528> Add a description. src/status_update_manager/status_update_manager_process.hpp Lines 87 (patched) <https://reviews.apache.org/r/64095/#comment270530> s/state/streams/ src/status_update_manager/status_update_manager_process.hpp Lines 119 (patched) <https://reviews.apache.org/r/64095/#comment270533> s/getPath/_getPath/ src/status_update_manager/status_update_manager_process.hpp Lines 122 (patched) <https://reviews.apache.org/r/64095/#comment270539> s/forward/_forwardCallback/ src/status_update_manager/status_update_manager_process.hpp Lines 130-132 (patched) <https://reviews.apache.org/r/64095/#comment270540> s/If no stream ID/If checkpoint is `false`/ src/status_update_manager/status_update_manager_process.hpp Lines 172-173 (patched) <https://reviews.apache.org/r/64095/#comment270543> Keep just the beginning. src/status_update_manager/status_update_manager_process.hpp Lines 178-179 (patched) <https://reviews.apache.org/r/64095/#comment270545> Forward the status update if this is at the front of the queue. Subsequent status updates will be sent in 'acknowledgement()'. src/status_update_manager/status_update_manager_process.hpp Lines 223-235 (patched) <https://reviews.apache.org/r/64095/#comment270553> Move this to Stream::acknowledgement src/status_update_manager/status_update_manager_process.hpp Lines 246 (patched) <https://reviews.apache.org/r/64095/#comment270554> s/Duplicate status acknowledgement/Duplicate status update acknowledgement/ src/status_update_manager/status_update_manager_process.hpp Lines 276 (patched) <https://reviews.apache.org/r/64095/#comment270556> Maybe "Recovers the status update manager's state using the supplied stream IDs." src/status_update_manager/status_update_manager_process.hpp Lines 280-281 (patched) <https://reviews.apache.org/r/64095/#comment270559> - The recovered state if successful. - The recovered state, including the number of errors encountered, if `strict == false` and any of the streams couldn't be recovered. - A `Failure` if `strict == true` and any of the streams couldn't be recovered. src/status_update_manager/status_update_manager_process.hpp Lines 288 (patched) <https://reviews.apache.org/r/64095/#comment270563> Let's call this just `state`. src/status_update_manager/status_update_manager_process.hpp Lines 307 (patched) <https://reviews.apache.org/r/64095/#comment270569> We want to increment `state.errors` if `strict=false`. src/status_update_manager/status_update_manager_process.hpp Lines 309 (patched) <https://reviews.apache.org/r/64095/#comment270567> // This can happen if the initial checkpoint of the stream didn't complete. src/status_update_manager/status_update_manager_process.hpp Lines 332 (patched) <https://reviews.apache.org/r/64095/#comment270571> s/the// src/status_update_manager/status_update_manager_process.hpp Lines 334 (patched) <https://reviews.apache.org/r/64095/#comment270572> I think that we don't need this one ;-)/ src/status_update_manager/status_update_manager_process.hpp Lines 341-342 (patched) <https://reviews.apache.org/r/64095/#comment270573> ``` LOG(INFO) << "Closing status update streams for framework" << " '" << frameworkId << "'"; ``` src/status_update_manager/status_update_manager_process.hpp Lines 346 (patched) <https://reviews.apache.org/r/64095/#comment270574> Split onto two different lines. src/status_update_manager/status_update_manager_process.hpp Lines 366-373 (patched) <https://reviews.apache.org/r/64095/#comment270575> We can use `stream->next()` here. src/status_update_manager/status_update_manager_process.hpp Lines 569 (patched) <https://reviews.apache.org/r/64095/#comment270568> We can make this a `bool` called `error`. src/status_update_manager/status_update_manager_process.hpp Lines 724 (patched) <https://reviews.apache.org/r/64095/#comment270570> If `state.updates.empty()` return `None()`. - Gaston Kleiman On Nov. 30, 2017, 5:42 p.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64095/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2017, 5:42 p.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 > >
