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 22:18:39 GMT

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




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

    Actually, after looking at this again I was thinking: perhaps this comment should be changed
to tell readers that we are doing two things here: both building our in-memory streams and
the state object which we return to the caller. WDYT?



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

    Could we use a `switch` here instead of `if`?



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

    s/.get()./->/



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

    As written, this is a bit opaque. I had to look up why we're seeking from the current
position with an offset of zero. I now see that we're doing this in an attempt to return the
current file position, ensuring that the current position is valid.
    
    Could you clarify this? I could imagine doing it by renaming this variable from `lseek`
to `currentPosition`, or putting a note in the comment above.



src/status_update_manager/status_update_manager_process.hpp
Lines 709-710 (patched)
<https://reviews.apache.org/r/64095/#comment270771>

    Fits on one line.



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

    Could you get rid of this `else`?



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

    We could probably use a `std::pair` here instead. And maybe we can use `std::make_pair`,
as long as it doesn't have issues with type deduction?



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

    s/duplicate/duplicate or has already been acknowledged/



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

    s/uuid/status_uuid/



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

    Could you remove the exclamation point?



src/status_update_manager/status_update_manager_process.hpp
Lines 780-783 (patched)
<https://reviews.apache.org/r/64095/#comment270787>

    I know that in this context, using just `uuid` is not as ambiguous, but given the presence
of multiple UUIDs in general I think it's better to be explicit. Could we use `status_uuid`
for the function argument?



src/status_update_manager/status_update_manager_process.hpp
Lines 790-791 (patched)
<https://reviews.apache.org/r/64095/#comment270790>

    Since we print the status UUID in the stream operator for the update type, we should be
able to eliminate the explicit UUID in the log line here and not lose any information.



src/status_update_manager/status_update_manager_process.hpp
Lines 847-849 (patched)
<https://reviews.apache.org/r/64095/#comment270810>

    I think it's more common for us to keep the empty function body on the same line:
    
    ```
    : terminated(false), streamId(_streamId), path(_path), fd(_fd) {}
    ```
    
    Here and elsewhere.



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

    s/its/it's/



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

    Ditto - use a `switch` here.



src/status_update_manager/status_update_manager_process.hpp
Lines 885-886 (patched)
<https://reviews.apache.org/r/64095/#comment270815>

    Hmm this error message seems strange in the ACK case?



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

    Ditto - use a `switch` here.



src/status_update_manager/status_update_manager_process.hpp
Lines 907-908 (patched)
<https://reviews.apache.org/r/64095/#comment270823>

    Let's pass framework ID into the creation method to eliminate the NONE case here.



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

    I think we can remove this comment.



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

    Can remove this comment.


- 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