mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.
Date Fri, 24 May 2019 15:59:18 GMT

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



Overall looking pretty good, thanks! Can you be sure to run the test in repetition to ensure
there's no flakyness?


src/tests/master/update_framework_tests.cpp
Lines 88 (patched)
<https://reviews.apache.org/r/70534/#comment302217>

    {}; can go on the previous line



src/tests/master/update_framework_tests.cpp
Lines 93 (patched)
<https://reviews.apache.org/r/70534/#comment302213>

    It seems strange to pass the `id` here.. can't we use the one in info and ensure that
callers set it?



src/tests/master/update_framework_tests.cpp
Lines 98-103 (patched)
<https://reviews.apache.org/r/70534/#comment302212>

    In general I would suggest we prefer the following syntax pattern:
    
    ```
     *call.mutable_framework_id() = id;
     *call.mutable_update_framework()->mutable_framework_info() = info;
     *call.mutable_update_framework()->mutable_framework_info()
       ->mutable_id() = id;
    ```
    
    Because it allows moves to happen when the rhs is an rvalue:
    
    ```
    *msg.mutable_foo() = func(); // will move :)
    msg.mutable_foo()->CopyFrom(func()); // will copy :(
    ```



src/tests/master/update_framework_tests.cpp
Lines 124 (patched)
<https://reviews.apache.org/r/70534/#comment302214>

    pull this up into the previous line?



src/tests/master/update_framework_tests.cpp
Lines 131 (patched)
<https://reviews.apache.org/r/70534/#comment302215>

    newline



src/tests/master/update_framework_tests.cpp
Lines 134 (patched)
<https://reviews.apache.org/r/70534/#comment302216>

    newline



src/tests/master/update_framework_tests.cpp
Lines 145 (patched)
<https://reviews.apache.org/r/70534/#comment302218>

    s/C/c/



src/tests/master/update_framework_tests.cpp
Lines 147-149 (patched)
<https://reviews.apache.org/r/70534/#comment302211>

    How about a CHECK_EQ with a helpful `<<` message so that the reader knows why this
is failing from the logs without having to come in here and read the code?



src/tests/master/update_framework_tests.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/70534/#comment302219>

    s/FrameworkInfosDiff/diff/?



src/tests/master/update_framework_tests.cpp
Lines 182 (patched)
<https://reviews.apache.org/r/70534/#comment302220>

    This seems odd, can't the caller fill this in for the initial FrameworkInfo so that we
don't ignore it? if the `id` is set to two different values, we want to catch that.



src/tests/master/update_framework_tests.cpp
Lines 197-198 (patched)
<https://reviews.apache.org/r/70534/#comment302221>

    Can you declare this lower down closer to where it's used? We can also pass in the DEFAULT_FRAMEWORK_INFO
**with** `id` assigned based on the subscription.
    
    Ditto for the other tests



src/tests/master/update_framework_tests.cpp
Lines 219 (patched)
<https://reviews.apache.org/r/70534/#comment302222>

    newline, ditto for the other tests



src/tests/master/update_framework_tests.cpp
Lines 230 (patched)
<https://reviews.apache.org/r/70534/#comment302236>

    newline, ditto other tests



src/tests/master/update_framework_tests.cpp
Lines 325 (patched)
<https://reviews.apache.org/r/70534/#comment302237>

    A role not in the whitelist? Don't we have a test for that somewhere already? Or are you
thinking specifically when an UPDATE_FRAMEWORK uses a non-whitelisted role?



src/tests/master/update_framework_tests.cpp
Lines 326 (patched)
<https://reviews.apache.org/r/70534/#comment302238>

    Ditto here, I guess we test this already somewhere (maybe) but this is just to make sure
the UPDATE_FRAMEWORK path goes throug that check?



src/tests/master/update_framework_tests.cpp
Lines 332 (patched)
<https://reviews.apache.org/r/70534/#comment302239>

    exit code?



src/tests/master/update_framework_tests.cpp
Lines 391 (patched)
<https://reviews.apache.org/r/70534/#comment302234>

    remove this?



src/tests/master/update_framework_tests.cpp
Lines 397 (patched)
<https://reviews.apache.org/r/70534/#comment302235>

    newline



src/tests/master/update_framework_tests.cpp
Lines 417 (patched)
<https://reviews.apache.org/r/70534/#comment302224>

    Can you explain what this is testing?



src/tests/master/update_framework_tests.cpp
Lines 421 (patched)
<https://reviews.apache.org/r/70534/#comment302223>

    newline



src/tests/master/update_framework_tests.cpp
Lines 424-425 (patched)
<https://reviews.apache.org/r/70534/#comment302228>

    See my comment below, it seems we don't need this?



src/tests/master/update_framework_tests.cpp
Lines 429-430 (patched)
<https://reviews.apache.org/r/70534/#comment302226>

    Why is this customization needed?



src/tests/master/update_framework_tests.cpp
Lines 434-438 (patched)
<https://reviews.apache.org/r/70534/#comment302227>

    This looks racy?
    
    The expectation should be set up prior to the agent starting, but do we even care to test
that there's an event for it in this test? It seems to be the responsibility of other tests
to care about agent lifecycle events.



src/tests/master/update_framework_tests.cpp
Lines 454 (patched)
<https://reviews.apache.org/r/70534/#comment302229>

    For consistency, we put the .WillOnce, .Times, etc on the next line, can you do a sweep
in this file?



src/tests/master/update_framework_tests.cpp
Lines 456 (patched)
<https://reviews.apache.org/r/70534/#comment302230>

    I hadn't considered the case where a framework has no roles, I suppose we can allow that..



src/tests/master/update_framework_tests.cpp
Lines 469 (patched)
<https://reviews.apache.org/r/70534/#comment302232>

    newline



src/tests/master/update_framework_tests.cpp
Lines 472 (patched)
<https://reviews.apache.org/r/70534/#comment302231>

    s/.get()./->/ (please do a scan of this file)
    
    Seems like 1 offer is expected given the single rescind expectation below, might as well
ensure it's 1 here?



src/tests/master/update_framework_tests.cpp
Lines 499 (patched)
<https://reviews.apache.org/r/70534/#comment302233>

    Pass `masterFlags.allocation_interval` instead


- Benjamin Mahler


On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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