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 69158: Added an integration test for resource provider removal.
Date Thu, 17 Jan 2019 11:17:28 GMT


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10755 (patched)
> > <https://reviews.apache.org/r/69158/diff/6/?file=2119934#file2119934line10755>
> >
> >     We can use a const reference here: `const v1::ResourceProviderID&`

Moved down to first use.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10834 (patched)
> > <https://reviews.apache.org/r/69158/diff/6/?file=2119934#file2119934line10834>
> >
> >     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?

Hmm. The other uses of `ContentType`s have no relation to the one here and do not need to
be identical. We need all entities using `contentType` to use the same value though. Dropping.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10895-10902 (patched)
> > <https://reviews.apache.org/r/69158/diff/6/?file=2119934#file2119934line10895>
> >
> >     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.

Yes, with the way the agent handler for `ResourceProviderMessage::Type::REMOVE` is written
we currently always send the status update before the agent resource update. Removed this
extra test. Like you write, should we ever break this invariant this test would already fail.

I added a comment to `src/slave.cpp` in https://reviews.apache.org/r/68147 to make this more
explicit.


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10924-10926 (patched)
> > <https://reviews.apache.org/r/69158/diff/6/?file=2119934#file2119934line10924>
> >
> >     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.

Good point about settling the clock.

While I agree that a test for "X never happens" can never be 100% certain (one would need
to wait forever to test "never"), it sets up a falsifiable model for the test which adds value
(a single failure would be enough to prove that the test assumption are incorrect; see K.
Popper et al. :D ).


> On Jan. 17, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 10946 (patched)
> > <https://reviews.apache.org/r/69158/diff/6/?file=2119934#file2119934line10946>
> >
> >     Let's check that the operation status has an agent ID but no resource provider
ID.

Very good point. Adding this check uncovered that we didn't set the agent ID in the status
update like noted by you here, https://reviews.apache.org/r/68147/#comment297704.


- Benjamin


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


On Jan. 17, 2019, 12:17 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69158/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2019, 12:17 p.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/7/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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