----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64095/#review192904 -----------------------------------------------------------
src/status_update_manager/status_update_manager_process.hpp Lines 56-58 (patched) <https://reviews.apache.org/r/64095/#comment271247> Should we add "Recovering status update state after failover" to this list? Also, should we explicitly say that the SUM is not responsible for garbage collection of completed streams? src/status_update_manager/status_update_manager_process.hpp Lines 67 (patched) <https://reviews.apache.org/r/64095/#comment271368> After looking at the recovery code, looks like we should replace: s/immediately after recovery/during recovery/ src/status_update_manager/status_update_manager_process.hpp Lines 73-74 (patched) <https://reviews.apache.org/r/64095/#comment271365> I did a quick search and it looks like we usually indent 4 spaces for template parameters? Could you confirm and update if appropriate? src/status_update_manager/status_update_manager_process.hpp Lines 95 (patched) <https://reviews.apache.org/r/64095/#comment271249> Add a comment explaining this member? src/status_update_manager/status_update_manager_process.hpp Lines 110-111 (patched) <https://reviews.apache.org/r/64095/#comment271250> Place on same line as initialization list. src/status_update_manager/status_update_manager_process.hpp Lines 115-120 (patched) <https://reviews.apache.org/r/64095/#comment271254> Is this code necessary? src/status_update_manager/status_update_manager_process.hpp Lines 128 (patched) <https://reviews.apache.org/r/64095/#comment271271> s/a call-back// src/status_update_manager/status_update_manager_process.hpp Lines 152 (patched) <https://reviews.apache.org/r/64095/#comment271302> Is this the appropriate place for this comment? Perhaps it can be removed? src/status_update_manager/status_update_manager_process.hpp Lines 182 (patched) <https://reviews.apache.org/r/64095/#comment271304> s/frameworkId/`frameworkId`/ or s/frameworkId/framework ID/ src/status_update_manager/status_update_manager_process.hpp Lines 206 (patched) <https://reviews.apache.org/r/64095/#comment271299> I don't understand this comment? src/status_update_manager/status_update_manager_process.hpp Lines 227 (patched) <https://reviews.apache.org/r/64095/#comment271307> Backticks instead of quotes for consistency. src/status_update_manager/status_update_manager_process.hpp Lines 237-238 (patched) <https://reviews.apache.org/r/64095/#comment271310> Maybe we should leave a TODO either here or next to the constant declaration saying that we should move this constant into this file once MESOS-8296 is resolved? src/status_update_manager/status_update_manager_process.hpp Lines 268 (patched) <https://reviews.apache.org/r/64095/#comment271319> s/the status update stream for stream/status update stream/ src/status_update_manager/status_update_manager_process.hpp Lines 268-269 (patched) <https://reviews.apache.org/r/64095/#comment271320> Indent 4 spaces. src/status_update_manager/status_update_manager_process.hpp Lines 285 (patched) <https://reviews.apache.org/r/64095/#comment271323> Can probably remove this comment. src/status_update_manager/status_update_manager_process.hpp Lines 311 (patched) <https://reviews.apache.org/r/64095/#comment271339> Period. src/status_update_manager/status_update_manager_process.hpp Lines 331-332 (patched) <https://reviews.apache.org/r/64095/#comment271340> The following is more consistent with our style guide: ``` const std::string message = "Failed to recover status update stream " + stringify(streamId) + ": " + result.error(); ``` src/status_update_manager/status_update_manager_process.hpp Lines 405 (patched) <https://reviews.apache.org/r/64095/#comment271346> Are we sure that this is being resent? Isn't it possible that the update was queued while the manager was paused? src/status_update_manager/status_update_manager_process.hpp Lines 429-432 (patched) <https://reviews.apache.org/r/64095/#comment271290> 4 Space indent src/status_update_manager/status_update_manager_process.hpp Lines 431 (patched) <https://reviews.apache.org/r/64095/#comment271350> Let's not use the C-style cast here. How about: ``` checkpoint ? Option<std::string>(getPath(streamId)) : None()); ``` I think you'll need to use `Option<std::string>::none()` for the NONE if the compiler complains. src/status_update_manager/status_update_manager_process.hpp Lines 454-457 (patched) <https://reviews.apache.org/r/64095/#comment271363> This indentation is difficult to parse. How about ``` Result<std::pair< process::Owned<StatusUpdateStream>, typename StatusUpdateStream::State>> result = StatusUpdateStream::recover(streamId, getPath(streamId), strict); ``` src/status_update_manager/status_update_manager_process.hpp Lines 496 (patched) <https://reviews.apache.org/r/64095/#comment271265> Let's return an `Option<StatusUpdateStream*>` here. See this related Slack discussion: https://mesos.slack.com/archives/C1LPTK50T/p1512502377000080 src/status_update_manager/status_update_manager_process.hpp Lines 497 (patched) <https://reviews.apache.org/r/64095/#comment271367> Indentation. src/status_update_manager/status_update_manager_process.hpp Lines 536 (patched) <https://reviews.apache.org/r/64095/#comment271369> Backticks instead of quotes for consistency. src/status_update_manager/status_update_manager_process.hpp Lines 539 (patched) <https://reviews.apache.org/r/64095/#comment271370> Is this comment necessary? I think the entire class should only be used for messages which expect an ACK? src/status_update_manager/status_update_manager_process.hpp Lines 551-558 (patched) <https://reviews.apache.org/r/64095/#comment271371> Indent only four spaces. src/status_update_manager/status_update_manager_process.hpp Lines 565 (patched) <https://reviews.apache.org/r/64095/#comment271374> You can probably remove this TODO. src/status_update_manager/status_update_manager_process.hpp Lines 573-587 (patched) <https://reviews.apache.org/r/64095/#comment271376> Sorry, after looking at this again I feel like we should switch to just sending the single update associated with the delay that has elapsed. Should be pretty simple to pass a stream ID into this? src/status_update_manager/status_update_manager_process.hpp Lines 593-594 (patched) <https://reviews.apache.org/r/64095/#comment271377> Are these used? src/status_update_manager/status_update_manager_process.hpp Lines 631-633 (patched) <https://reviews.apache.org/r/64095/#comment271381> Indentation. src/status_update_manager/status_update_manager_process.hpp Lines 640 (patched) <https://reviews.apache.org/r/64095/#comment271384> Indentation. I'm gonna stop raising issues for these :) Please do a sweep and fix indentation throughout the chain. src/status_update_manager/status_update_manager_process.hpp Lines 671 (patched) <https://reviews.apache.org/r/64095/#comment271385> As we discussed, you should be able to `return std::move(stream);` here. src/status_update_manager/status_update_manager_process.hpp Lines 715-725 (patched) <https://reviews.apache.org/r/64095/#comment271387> I'm having trouble understanding why we don't allow these errors during non-strict recovery, but we will allow errors during protobuf parsing? src/status_update_manager/status_update_manager_process.hpp Lines 739-740 (patched) <https://reviews.apache.org/r/64095/#comment271386> Backticks for consistency, here and below. src/status_update_manager/status_update_manager_process.hpp Lines 758-759 (patched) <https://reviews.apache.org/r/64095/#comment271388> Ditto regarding formatting here. src/status_update_manager/status_update_manager_process.hpp Lines 777 (patched) <https://reviews.apache.org/r/64095/#comment271389> Remove extra space. src/status_update_manager/status_update_manager_process.hpp Lines 791 (patched) <https://reviews.apache.org/r/64095/#comment271390> s/duplicate/is a duplicate/ src/status_update_manager/status_update_manager_process.hpp Lines 873-874 (patched) <https://reviews.apache.org/r/64095/#comment271391> ``` if (statusUuid != UUID::fromBytes(update.status().status_uuid()).get()) { ``` src/status_update_manager/status_update_manager_process.hpp Lines 906 (patched) <https://reviews.apache.org/r/64095/#comment271392> Remove extra space. src/status_update_manager/status_update_manager_process.hpp Lines 987 (patched) <https://reviews.apache.org/r/64095/#comment271394> Capitalize "This", here and elsewhere. src/status_update_manager/status_update_manager_process.hpp Lines 1026 (patched) <https://reviews.apache.org/r/64095/#comment271393> s/MANAGER/MANAGER_PROCESS/ - Greg Mann On Dec. 2, 2017, 2:44 a.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64095/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2017, 2:44 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/7/ > > > Testing > ------- > > Added new tests at the end of the chain. They passed 5000 times on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
