You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/05/02 16:04:44 UTC
Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 2, 2019, 4:04 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added tests for updates sent by the master to slaves and API subscribers.
Summary (updated)
-----------------
Added tests for the UPDATE_FRAMEWORK call.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description (updated)
-------
This patch adds tests for the UPDATE_FRAMEWORK call.
The reason behind implementing the MasterAPISubscriber class with mock methods is the need to wait for interesting API events while skipping uninteresting ones.
This is needed to properly test the FRAMEWORK_UPDATED event sent by the master (for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip everything else).
I'm in doubts about the proper location for the MasterAPISubscriber. On one hand, it is used only by these tests and the implementation is by no means complete; on the other hand, this might be useful in other tests of the master API behaviour.
Probably, turning the MasterAPISubscriber into a general-purpose mock (and moving it into another file at that point) should be a separate task.
Diffs (updated)
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/update_framework_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70534/diff/2/
Changes: https://reviews.apache.org/r/70534/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215335
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70666, 70667, 70668, 70669, 70670, 70533, 70671, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 17, 2019, 8:19 a.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 17, 2019, 8:19 a.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/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 417 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line417>
> >
> > Can you explain what this is testing?
Split this test into two, tried to come up with better names for them and added a comment to each - I hope now it is more readable.
> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 429-430 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line429>
> >
> > Why is this customization needed?
Not needed at all - removed it. Also tried to remove other remaining customization wherever possible.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215500
-----------------------------------------------------------
On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 5:36 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/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 434-438 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line434>
> >
> > 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.
Indeed, this is misplaced and has at least two races: a high-level one (setting expectation vs the actual event) and a low-level one (calling EXPECT_CALL vs calling the mock method). I'll fix this and take care of similar problems elsewhere in these patches.
The intention behind this AWAIT was to keep a fixed ordering between `allocator->addSlave()` and `allocator->updateFramework()`.
I do not think it is a good idea to have a test in which these two calls occur in some arbitrary (not random, but arbitrary!) order.
Currently only the case "update after a slave is added" is tested. If we need to test a second case, it probably makes sense to have a second test.
I'm also not sure if waiting for AGENT_ADDED is the best way to ensure the ordering (waiting for dispatch on `MesosAllocatorProcess::addSlave` could probably be another option).
Even if we settle on waiting for AGENT_ADDED, I'll have to write a better comment.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215500
-----------------------------------------------------------
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
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215460
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215411
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215774
-----------------------------------------------------------
Fix it, then Ship it!
Nice tests, thank you Andrei!
src/tests/master/update_framework_tests.cpp
Lines 150 (patched)
<https://reviews.apache.org/r/70534/#comment302617>
s/update/newInfo/
src/tests/master/update_framework_tests.cpp
Lines 150 (patched)
<https://reviews.apache.org/r/70534/#comment302618>
how about: s/update/newInfo/
src/tests/master/update_framework_tests.cpp
Lines 157-158 (patched)
<https://reviews.apache.org/r/70534/#comment302614>
This seems a little brittle, as we probably should be failing FrameworkInfo validation when there is an UNKNOWN capability. Seems like a bug that it's not getting checked
How about REGION_AWARE?
src/tests/master/update_framework_tests.cpp
Lines 164-169 (patched)
<https://reviews.apache.org/r/70534/#comment302615>
Remove "without_resources"? I'm not sure what it's trying to communicate, but it seems to carry too much context from somewhere else, is it named this way because of a test?
src/tests/master/update_framework_tests.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/70534/#comment302619>
newline
src/tests/master/update_framework_tests.cpp
Lines 174 (patched)
<https://reviews.apache.org/r/70534/#comment302620>
s/std::string/string/ here and below
src/tests/master/update_framework_tests.cpp
Lines 274 (patched)
<https://reviews.apache.org/r/70534/#comment302621>
As an aside, the error above says "Updating" and this one says "Changing", should probably make the language consistent
src/tests/master/update_framework_tests.cpp
Lines 370-375 (patched)
<https://reviews.apache.org/r/70534/#comment302622>
This looks racy..?
The agent may get added before the subscriber subscribes, in which case the AGENT_ADDED event wouldn't be sent because the agent gets included in the initial SUBSCRIBE state.
src/tests/master/update_framework_tests.cpp
Lines 493 (patched)
<https://reviews.apache.org/r/70534/#comment302624>
Why resume and re-pause the clock?
src/tests/master/update_framework_tests.cpp
Lines 512 (patched)
<https://reviews.apache.org/r/70534/#comment302625>
Why resume?
src/tests/master/update_framework_tests.cpp
Lines 562-565 (patched)
<https://reviews.apache.org/r/70534/#comment302631>
```
Future<Event::Rescind> rescind;
EXPECT_CALL(*scheduler, rescind(_, _))
.WillOnce(FutureArg<1>(&rescind));
```
src/tests/master/update_framework_tests.cpp
Lines 569-570 (patched)
<https://reviews.apache.org/r/70534/#comment302629>
```
// TODO(asekretenko): Add a more in-depth check that the
// allocator does what it should.
```
- Benjamin Mahler
On June 3, 2019, 8:14 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated June 3, 2019, 8:14 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/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated June 3, 2019, 8:14 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 (updated)
-----
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/9/
Changes: https://reviews.apache.org/r/70534/diff/8-9/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215592
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 5:36 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/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215553
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 5:36 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/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 28, 2019, 5:36 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 (updated)
-----
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/8/
Changes: https://reviews.apache.org/r/70534/diff/7-8/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/70534/diff/6-7/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 21, 2019, 1:56 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 (updated)
-----
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/6/
Changes: https://reviews.apache.org/r/70534/diff/5-6/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 17, 2019, 3:19 p.m.)
Review request for mesos and Benjamin Mahler.
Summary (updated)
-----------------
Added tests for the V1 UPDATE_FRAMEWORK call.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description (updated)
-------
Added tests for the V1 UPDATE_FRAMEWORK call.
Diffs (updated)
-----
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/5/
Changes: https://reviews.apache.org/r/70534/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215191
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 7, 2019, 12:13 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 UPDATE_FRAMEWORK call.
>
>
> Diffs
> -----
>
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70534/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215089
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 7, 2019, 12:13 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 UPDATE_FRAMEWORK call.
>
>
> Diffs
> -----
>
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70534/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK
call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 98 (patched)
> > <https://reviews.apache.org/r/70534/diff/4/?file=2143581#file2143581line98>
> >
> > What does the presence of agent reservations matter to testing UPDATE_FRAMEWORK?
Indeed, reservations are not necessary for testing that allocator gets the update.
We can update from no roles to one role and check for an offer - I rewrote the test.
> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 390-421 (patched)
> > <https://reviews.apache.org/r/70534/diff/4/?file=2143581#file2143581line390>
> >
> > Is there no mock for this yet? How was the event streaming API tested without it?
> >
> > How about pulling this out in front of this patch into its own header so that we can review it on its own? It's independent and adds to the complexity of this review.
It was tested by explicitly reading out every event (example: https://github.com/apache/mesos/blob/91fa416bf0d6191bb1a5e1fa3c883e23c98e892c/src/tests/api_tests.cpp#L3396).
Surely, each time a new event type was introduced, one had to modify unrelated tests: https://reviews.apache.org/r/61262/diff/8#index_header
Moved this mock into https://reviews.apache.org/r/70671/
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215206
-----------------------------------------------------------
On May 17, 2019, 3:19 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 17, 2019, 3:19 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/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215206
-----------------------------------------------------------
Thanks for the tests!
Having a test file just for a specific scheduler::Call doesn't fit our existing tests well. It seems reasonable to have its own file, but we could badly use some organization. This seems to be a "master test" but it does seem nice to cleanly have its own file.
How about a tests/master/ folder under which we move the master tests, as well as this test?
src/tests/update_framework_tests.cpp
Lines 88-94 (patched)
<https://reviews.apache.org/r/70534/#comment301791>
These types of functions lead to low readability. When reading tests, I can't intuit what this function does and I need to read the code to know that it puts in a special role.
In general, if we can't come up with a function that has some clear meaning, we will inline the code. This means that while we will have duplication, the test will be more readable (the reader doesn't need to jump up to this function and read what it does to understand the test).
src/tests/update_framework_tests.cpp
Lines 96-102 (patched)
<https://reviews.apache.org/r/70534/#comment301794>
We generally try to avoid such heavy context test fixtures if possible, since they reduce readability.
The reader of any of the tests below needs a lot of context about what the fixture has set up to understand how the test works.
If possible it's better to compose pieces together to achieve the test.
For example, I imagine the whitelist is only relevant to a few of the tests, so no need to have it for all of them.
The agent reservations, I'm not quite sure how they're relevant, there's no indication of why we need them here so I would have to read all the tests to find out.
The scheduler, well, we can just use it in each test, no need to have a fixture for it.
src/tests/update_framework_tests.cpp
Lines 98 (patched)
<https://reviews.apache.org/r/70534/#comment301793>
What does the presence of agent reservations matter to testing UPDATE_FRAMEWORK?
src/tests/update_framework_tests.cpp
Lines 177 (patched)
<https://reviews.apache.org/r/70534/#comment301792>
On the other hand, this function seems rather intuitive. I could figure out what it does from the call site. However.. I would get a bit confused from the fact that it returns a single FrameworkInfo, since the master can have several.
How about returning a vector of FrameworkInfo here and the caller ASSERTs that its size is 1.
src/tests/update_framework_tests.cpp
Lines 390-421 (patched)
<https://reviews.apache.org/r/70534/#comment301795>
Is there no mock for this yet? How was the event streaming API tested without it?
How about pulling this out in front of this patch into its own header so that we can review it on its own? It's independent and adds to the complexity of this review.
src/tests/update_framework_tests.cpp
Lines 477 (patched)
<https://reviews.apache.org/r/70534/#comment301796>
Do you have a bad indentation setting in your code editor? There seem to be a lot of 4 space indents in this chain of patches
- Benjamin Mahler
On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 7, 2019, 12:13 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 UPDATE_FRAMEWORK call.
>
>
> Diffs
> -----
>
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70534/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 7, 2019, 12:13 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added a comment and TODO for the MasterAPISubscriber based on the review description; cleared the description.
Summary (updated)
-----------------
Added tests for the UPDATE_FRAMEWORK call.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description (updated)
-------
Added tests for the UPDATE_FRAMEWORK call.
Diffs (updated)
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/update_framework_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70534/diff/4/
Changes: https://reviews.apache.org/r/70534/diff/3-4/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/#review215042
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 2, 2019, 4:06 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
>
> (Updated May 2, 2019, 4:06 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds tests for the UPDATE_FRAMEWORK call.
>
> The reason behind implementing the MasterAPISubscriber class with mock methods is the need to wait for interesting API events while skipping uninteresting ones.
> This is needed to properly test the FRAMEWORK_UPDATED event sent by the master (for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip everything else).
>
> I'm in doubts about the proper location for the MasterAPISubscriber. On one hand, it is used only by these tests and the implementation is by no means complete; on the other hand, this might be useful in other tests of the master API behaviour.
> Probably, turning the MasterAPISubscriber into a general-purpose mock (and moving it into another file at that point) should be a separate task.
>
>
> Diffs
> -----
>
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70534/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70534/
-----------------------------------------------------------
(Updated May 2, 2019, 4:06 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added tests for updates sent by the master to slaves and API subscribers.
Summary (updated)
-----------------
Added tests for UPDATE_FRAMEWORK.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch adds tests for the UPDATE_FRAMEWORK call.
The reason behind implementing the MasterAPISubscriber class with mock methods is the need to wait for interesting API events while skipping uninteresting ones.
This is needed to properly test the FRAMEWORK_UPDATED event sent by the master (for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip everything else).
I'm in doubts about the proper location for the MasterAPISubscriber. On one hand, it is used only by these tests and the implementation is by no means complete; on the other hand, this might be useful in other tests of the master API behaviour.
Probably, turning the MasterAPISubscriber into a general-purpose mock (and moving it into another file at that point) should be a separate task.
Diffs (updated)
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/update_framework_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70534/diff/3/
Changes: https://reviews.apache.org/r/70534/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko