mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 64095: Added a generic actor to be used by status update managers.
Date Wed, 06 Dec 2017 04:22:53 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message