mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 70519: Transitioned tasks when an unreachable agent is marked as gone.
Date Tue, 23 Apr 2019 16:57:26 GMT

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


Fix it, then Ship it!




The change itself looks good to me, so all that follows below are fairly minor style comments.

One thing that we might think about is if we can somehow abstract out the iteration over all
tasks itself, since it seems like it is being used in multiple places. However, I didn't really
have a good idea of how this should look like, so I did not make a separate comment about
that.


src/master/master.cpp
Lines 9225 (patched)
<https://reviews.apache.org/r/70519/#comment301066>

    It looks like the `MESOS-XXXX` still needs to be updated.



src/master/master.cpp
Lines 9230 (patched)
<https://reviews.apache.org/r/70519/#comment301067>

    Nit: I'd probably write this as
    
        CHECK(slaves.recovered.contains(slaveId) || slaves.unreachable.contains(slaveId))
        
    That way the code matches exactly what's written in the comment.



src/master/master.cpp
Lines 9243 (patched)
<https://reviews.apache.org/r/70519/#comment301071>

    Nit: We could save one level of indentation (and more importantly one item on the reader's
mental stack) by writing this as
    
        if (framework == nullptr) {
          continue;
        }



src/master/master.cpp
Lines 9255 (patched)
<https://reviews.apache.org/r/70519/#comment301072>

    The return type here looks very dangerous: A correct implementation of `Owned<>`
should destroy its contents when it goes out of scope.
    
    I'm a bit torn here because I know it works with our current implementation of `Owned<>`,
but it seems a bit odd to "exploit" the implementation details this way.
    
    Maybe we should use `.find()` or `operator[]` instead?



src/master/master.cpp
Lines 9282 (patched)
<https://reviews.apache.org/r/70519/#comment301074>

    The same comment as above also applies here, this seems to be leaning very heavily on
the fact that `Owned<T>` is in fact `shared_ptr<T>`.


- Benno Evers


On April 22, 2019, 11:57 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70519/
> -----------------------------------------------------------
> 
> (Updated April 22, 2019, 11:57 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Gastón Kleiman, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-9545
>     https://issues.apache.org/jira/browse/MESOS-9545
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the master code responsible for marking
> agents as gone to properly transition tasks on agents which
> were previously marked as unreachable.
> 
> 
> Diffs
> -----
> 
>   src/master/framework.cpp 05f5514c589b2dba08afe77281e5fbc4e29f232b 
>   src/master/http.cpp e7a92d0f554ba4cafaee5a75f09b46eb1bf4a310 
>   src/master/master.hpp 94891af9deeaddb3333fc9d6eabb243aed97f7b7 
>   src/master/master.cpp ad54ae217863a08f4e6d743b39c176b171353084 
>   src/tests/api_tests.cpp e76417a9098281265b3411c18767bfcc2f624b6f 
> 
> 
> Diff: https://reviews.apache.org/r/70519/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*UnreachableAgentMarkedGone*" --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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