mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gaston Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 64095: Added a generic actor to be used by status update managers.
Date Fri, 01 Dec 2017 01:42:41 GMT

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


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