mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 69158: Added an integration test for resource provider removal.
Date Thu, 17 Jan 2019 05:13:50 GMT

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




src/tests/slave_tests.cpp
Lines 10731 (patched)
<https://reviews.apache.org/r/69158/#comment297710>

    `v1::ResourceProviderInfo`



src/tests/slave_tests.cpp
Lines 10755 (patched)
<https://reviews.apache.org/r/69158/#comment297709>

    We can use a const reference here: `const v1::ResourceProviderID&`



src/tests/slave_tests.cpp
Lines 10834 (patched)
<https://reviews.apache.org/r/69158/#comment297711>

    If you think it is a good idea to create this alias, how about moving it to the beginning
so the above two occurrences of `ContentType::PROTOBUF` can all be replaced?



src/tests/slave_tests.cpp
Lines 10895-10902 (patched)
<https://reviews.apache.org/r/69158/#comment297712>

    Is the order guaranteed? If yes, it seems to me that it is sufficient to just verify that
the scheduler receives `OPERATION_GONE_BY_OPERATOR` right? The order validation here makes
the test a bit hard to follow, so I suggest just remove the check here, as well as the two
futures.
    
    If the order is not guaranteed, then it seems we have a problem to resolve.



src/tests/slave_tests.cpp
Lines 10924-10926 (patched)
<https://reviews.apache.org/r/69158/#comment297713>

    To make this check more reliable, we should add a `Clock::settle()` at the end of the
test.
    
    Even so, it cannot 100% verify the property to test since HTTP messages could be in transmission
when the test stops.



src/tests/slave_tests.cpp
Lines 10946 (patched)
<https://reviews.apache.org/r/69158/#comment297714>

    Let's check that the operation status has an agent ID but no resource provider ID.


- Chun-Hung Hsiao


On Jan. 16, 2019, 10:51 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2019, 10:51 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
>     https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added an integration test for resource provider removal.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp dc711de96053ebecb6b71b98f14f22cb9b050c52 
> 
> 
> Diff: https://reviews.apache.org/r/69158/diff/6/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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