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/29 18:22:20 UTC
Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs
-----
src/tests/master/update_framework_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70755/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 11, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 664-666 (patched)
> > <https://reviews.apache.org/r/70755/diff/2/?file=2147660#file2147660line664>
> >
> > Same race as the v1 tests, this needs to be before StartSlave, otherwise the agent might show up in the initial SUBSCRIBE state
Yeah, I've fixed it in `UpdateFrameworkTest.OffersOnAddingRole` and somehow forgot about three other places :(
> On June 11, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 690-691 (patched)
> > <https://reviews.apache.org/r/70755/diff/2/?file=2147660#file2147660line690>
> >
> > V0 scheduler event? I guess you mean the master acknowledging the update? We may never add that, so probably we should just remove the TODO
Yes, exactly. Removed this TODO.
> On June 11, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 714 (patched)
> > <https://reviews.apache.org/r/70755/diff/2/?file=2147660#file2147660line714>
> >
> > newline
> >
> > and maybe a comment that we sanity check that it's different from the default? seems a bit odd to check here as opposed to earlier when calling changeAllMutableFields?
After looking at the proliferating sanity checks for `ChangeAllMutableFields()`+`diff()`, I've realized that it will probably be better to replace them (and the CHECK for the count of fields in FrameworkInfo) with a simple unit test for this pair of helpers. This will follow in a separate patch.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215810
-----------------------------------------------------------
On June 12, 2019, 4:25 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated June 12, 2019, 4:25 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
>
>
> Diff: https://reviews.apache.org/r/70755/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 11, 2019, 7:36 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 780 (patched)
> > <https://reviews.apache.org/r/70755/diff/2/?file=2147660#file2147660line780>
> >
> > do you need to resume here for the test to pass?
Yes, it turns out we do. We need some allocation interval to start after the FrameworkInfo update is applied by the allocator.
The V1 test acheives this by advancing clock after the update is confirmed by the master.
... wait, now I begin to suspect that the V1 test without resume() has the same issue (albeit less prominent) - there the test can receive the update confirmation BEFORE the allocator process handles updateFramework. I'll need to address that separately.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215810
-----------------------------------------------------------
On June 12, 2019, 4:44 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated June 12, 2019, 4:44 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
>
>
> Diff: https://reviews.apache.org/r/70755/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215810
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/master/update_framework_tests.cpp
Lines 627 (patched)
<https://reviews.apache.org/r/70755/#comment302695>
.WillOnce on the next line
src/tests/master/update_framework_tests.cpp
Lines 654-655 (patched)
<https://reviews.apache.org/r/70755/#comment302696>
No need to create and pass in the agent flags? They're not being used below?
src/tests/master/update_framework_tests.cpp
Lines 664-666 (patched)
<https://reviews.apache.org/r/70755/#comment302698>
Same race as the v1 tests, this needs to be before StartSlave, otherwise the agent might show up in the initial SUBSCRIBE state
src/tests/master/update_framework_tests.cpp
Lines 690-691 (patched)
<https://reviews.apache.org/r/70755/#comment302699>
V0 scheduler event? I guess you mean the master acknowledging the update? We may never add that, so probably we should just remove the TODO
src/tests/master/update_framework_tests.cpp
Lines 696 (patched)
<https://reviews.apache.org/r/70755/#comment302700>
This comment seems a little confusing, it made me think that we're about to test that the ID set gets rejected, but instead it's just adding it for comparision purposes.
src/tests/master/update_framework_tests.cpp
Lines 714 (patched)
<https://reviews.apache.org/r/70755/#comment302701>
newline
and maybe a comment that we sanity check that it's different from the default? seems a bit odd to check here as opposed to earlier when calling changeAllMutableFields?
src/tests/master/update_framework_tests.cpp
Lines 749-750 (patched)
<https://reviews.apache.org/r/70755/#comment302704>
Ditto here.
src/tests/master/update_framework_tests.cpp
Lines 780 (patched)
<https://reviews.apache.org/r/70755/#comment302702>
do you need to resume here for the test to pass?
src/tests/master/update_framework_tests.cpp
Lines 795 (patched)
<https://reviews.apache.org/r/70755/#comment302703>
does s/1ul/1u/ not work? I think we do this in other tests
src/tests/master/update_framework_tests.cpp
Lines 809-810 (patched)
<https://reviews.apache.org/r/70755/#comment302705>
Ditto here.
src/tests/master/update_framework_tests.cpp
Lines 813-818 (patched)
<https://reviews.apache.org/r/70755/#comment302706>
```
MockScheduler sched;
// Expect an offer exactly once (after subscribing).
Future<std::vector<Offer>> offers;
```
src/tests/master/update_framework_tests.cpp
Lines 824 (patched)
<https://reviews.apache.org/r/70755/#comment302707>
newline
src/tests/master/update_framework_tests.cpp
Lines 825 (patched)
<https://reviews.apache.org/r/70755/#comment302708>
Ditto here.
src/tests/master/update_framework_tests.cpp
Lines 836-837 (patched)
<https://reviews.apache.org/r/70755/#comment302709>
// TODO(asekretenko): Add a more in-depth check that
// the allocator does what it should.
- Benjamin Mahler
On June 3, 2019, 8:16 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated June 3, 2019, 8:16 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70755/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215667
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534, 70751, 70752, 70753, 70754, 70755]
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 June 3, 2019, 8:16 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated June 3, 2019, 8:16 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70755/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215854
-----------------------------------------------------------
src/tests/master/update_framework_tests.cpp
Lines 696-697 (original), 693-695 (patched)
<https://reviews.apache.org/r/70755/#comment302791>
Now that I see this, it seems odd? `updateFramework()` should require that the ID is set, since it has been assigned by the time the caller calls this? (technically it's possible that it hasn't but that seems a very rare case we shouldn't worry about, since the scheduler can't have really done anything prior to getting an ID).
- Benjamin Mahler
On June 12, 2019, 6:47 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated June 12, 2019, 6:47 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
>
>
> Diff: https://reviews.apache.org/r/70755/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
(Updated June 12, 2019, 6:47 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Reverted deleting Clock::resume()
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs (updated)
-----
src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
Diff: https://reviews.apache.org/r/70755/diff/6/
Changes: https://reviews.apache.org/r/70755/diff/5-6/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
(Updated June 12, 2019, 4:44 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Reverted resume() removal.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs (updated)
-----
src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
Diff: https://reviews.apache.org/r/70755/diff/5/
Changes: https://reviews.apache.org/r/70755/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
(Updated June 12, 2019, 4:25 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs (updated)
-----
src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
Diff: https://reviews.apache.org/r/70755/diff/4/
Changes: https://reviews.apache.org/r/70755/diff/3-4/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
(Updated June 12, 2019, 4:14 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Changes
-------
Rebased; fixed issues.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs (updated)
-----
src/tests/master/update_framework_tests.cpp 78304c211c5eb9f50ea629121a6dddfe47942e9f
Diff: https://reviews.apache.org/r/70755/diff/3/
Changes: https://reviews.apache.org/r/70755/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/
-----------------------------------------------------------
(Updated June 3, 2019, 8:16 p.m.)
Review request for mesos, Benjamin Mahler and Meng Zhu.
Bugs: MESOS-9793
https://issues.apache.org/jira/browse/MESOS-9793
Repository: mesos
Description
-------
Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
Diffs (updated)
-----
src/tests/master/update_framework_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70755/diff/2/
Changes: https://reviews.apache.org/r/70755/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70755: Added tests for the V0 UPDATE_FRAMEWORK
scheduler call.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70755/#review215594
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534, 70751, 70752, 70753, 70754, 70755]
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 29, 2019, 6:22 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70755/
> -----------------------------------------------------------
>
> (Updated May 29, 2019, 6:22 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Meng Zhu.
>
>
> Bugs: MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9793
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the V0 UPDATE_FRAMEWORK scheduler call.
>
>
> Diffs
> -----
>
> src/tests/master/update_framework_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70755/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>