mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 63625: Added a test for resource conversion using a resource provider.
Date Tue, 07 Nov 2017 16:34:21 GMT

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




src/tests/resource_provider_manager_tests.cpp
Lines 730 (patched)
<https://reviews.apache.org/r/63625/#comment267626>

    Let's instead `ASSERT` on the value from `subscribed->provider_id()`.



src/tests/resource_provider_manager_tests.cpp
Lines 738-739 (patched)
<https://reviews.apache.org/r/63625/#comment267627>

    Maybe use a helper here?
    
        disk.mutable_disk()->mutable_source()->CopyFrom(createDiskSourceRaw());



src/tests/resource_provider_manager_tests.cpp
Lines 749 (patched)
<https://reviews.apache.org/r/63625/#comment267629>

    This does not guarantee that the allocator will offer the resources from the resource
provider in the next offer cycle since it might still be behind the master's view.
    
    We could instead e.g., reject decline all offers not containing the resource provider
resources below; maybe you have another idea.



src/tests/resource_provider_manager_tests.cpp
Lines 754 (patched)
<https://reviews.apache.org/r/63625/#comment267630>

    Might not need this for a single use below.



src/tests/resource_provider_manager_tests.cpp
Lines 817 (patched)
<https://reviews.apache.org/r/63625/#comment267631>

    We should add helpers for the new operations to `src/tests/mesos.hpp`, and use them here,
e.g.,
    
        mesos::v1::Offer::Operation = v1::CREATE_BLOCK(disk);



src/tests/resource_provider_manager_tests.cpp
Lines 828 (patched)
<https://reviews.apache.org/r/63625/#comment267632>

    Let's `ASSERT(operation->info().has_create_block())`.



src/tests/resource_provider_manager_tests.cpp
Lines 835 (patched)
<https://reviews.apache.org/r/63625/#comment267637>

    Can we run the whole test with paused clock? It seems that the allocator could still be
assembling an offer before we pause the clock so we do not get the expected offer below.



src/tests/resource_provider_manager_tests.cpp
Lines 858 (patched)
<https://reviews.apache.org/r/63625/#comment267634>

    `const` ref.



src/tests/resource_provider_manager_tests.cpp
Lines 867 (patched)
<https://reviews.apache.org/r/63625/#comment267636>

    `ASSERT_SOME(block)`.



src/tests/resource_provider_manager_tests.cpp
Lines 869 (patched)
<https://reviews.apache.org/r/63625/#comment267635>

    Break the line after `,`.


- Benjamin Bannier


On Nov. 7, 2017, 4:36 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63625/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2017, 4:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test for resource conversion using a resource provider.
> 
> 
> Diffs
> -----
> 
>   src/tests/resource_provider_manager_tests.cpp 4008b1c751d6227b99adef756e95174d7d8a62f2

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


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