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 Fri, 01 Dec 2017 19:54:13 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64095/#review192528
-----------------------------------------------------------




src/status_update_manager/status_update_manager_process.hpp
Lines 588 (patched)
<https://reviews.apache.org/r/64095/#comment270729>

    Could we switch to returning an `Owned` here instead of a raw pointer?



src/status_update_manager/status_update_manager_process.hpp
Lines 593-596 (patched)
<https://reviews.apache.org/r/64095/#comment270730>

    What are the implications of this for the empty file case? It seems the caller must be
aware that they should delete an empty file, but I'm not sure that they receive specific enough
feedback for this.
    
    Perhaps `recover` should remove the file if it's empty?



src/status_update_manager/status_update_manager_process.hpp
Lines 609-610 (patched)
<https://reviews.apache.org/r/64095/#comment270733>

    Is there a reason you're not using `O_APPEND` here?



src/status_update_manager/status_update_manager_process.hpp
Lines 650 (patched)
<https://reviews.apache.org/r/64095/#comment270746>

    You can probably remove this comment.



src/status_update_manager/status_update_manager_process.hpp
Lines 651 (patched)
<https://reviews.apache.org/r/64095/#comment270745>

    s/update stream/updates/



src/status_update_manager/status_update_manager_process.hpp
Lines 654 (patched)
<https://reviews.apache.org/r/64095/#comment270748>

    Given the log line above, I think this comment can be removed as well.


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


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