mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 70165: Fixed operator operation handling with resource provider resources.
Date Fri, 08 Mar 2019 14:37:06 GMT

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



Thanks for fixing this and adding a test!

I only did a partial review of the test code. I think we should

1) clearly group the different test stages and add comments explaining what we are doing and
2) distinguish `AWAIT` for synchronization from ones use to observe behavior under test (offers
with correct resources); printing a message for the latter would be helpful, e.g.,
````
AWAIT_READY(offers) << "Did not observe resource result for operation " << operation;
````


src/tests/storage_local_resource_provider_tests.cpp
Lines 5454 (patched)
<https://reviews.apache.org/r/70165/#comment299527>

    Let's add a comment that we use a hierarchical role so we can do a reservation refinement.



src/tests/storage_local_resource_provider_tests.cpp
Lines 5469 (patched)
<https://reviews.apache.org/r/70165/#comment299528>

    We usually would write
    ```
    EXPECT_CALL(sched, offerRescinded(_, _))
      .Times(4);
    ```
    
    I am not sure there's much value in adding an expectation _for exactly four calls_ here
since below we use dedicated matchers to detect exactly the resources we are interested in;
how often offers are rescinded isn't under test here.



src/tests/storage_local_resource_provider_tests.cpp
Lines 5473 (patched)
<https://reviews.apache.org/r/70165/#comment299529>

    It would be great if we had some grouping of related actions with a comment explaining
what we are doing.
    
    One way to do that would be to write related actions in a separate block
    ```
    // Perform stuff so we can do stuff.
    {
       // Stuff.
    }
    ```
    
    The group here performs a `CREATE_DISK` and spans the  lines 5473-5511.



src/tests/storage_local_resource_provider_tests.cpp
Lines 5513-5514 (patched)
<https://reviews.apache.org/r/70165/#comment299530>

    This observes just a precondition and could be left as is.



src/tests/storage_local_resource_provider_tests.cpp
Lines 5570-5571 (patched)
<https://reviews.apache.org/r/70165/#comment299531>

    This observes that the operation under test succeeded  which should be called out.


- Benjamin Bannier


On March 8, 2019, 2:34 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70165/
> -----------------------------------------------------------
> 
> (Updated March 8, 2019, 2:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9612
>     https://issues.apache.org/jira/browse/MESOS-9612
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The resource provider manager didn't allow operations originating from
> operator API calls. For speculatively applied operations, this is
> allowed now.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f 
>   src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f

> 
> 
> Diff: https://reviews.apache.org/r/70165/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


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