You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2017/01/10 14:50:30 UTC
Review Request 55381: Added test for framework upgrading to multi-role
capability.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Bugs: MESOS-6900
https://issues.apache.org/jira/browse/MESOS-6900
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
Diff: https://reviews.apache.org/r/55381/diff/
Testing
-------
make check (OS X)
Thanks,
Benjamin Bannier
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161367
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/tests/master_validation_tests.cpp (lines 2593 - 2596)
<https://reviews.apache.org/r/55381/#comment232608>
How about following to avoid a new line between for `add_capabilities`?
```
frameworkInfo.add_roles(frameworkInfo.role());
frameworkInfo.clear_role();
frameworkInfo.add_capabilities()->set_type(
FrameworkInfo::Capability::MULTI_ROLE);
```
src/tests/master_validation_tests.cpp (lines 2604 - 2605)
<https://reviews.apache.org/r/55381/#comment232606>
new line here
- Guangya Liu
On \u4e00\u6708 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 11, 2017, 10:37 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 19, 2017, 11:45 p.m., Benjamin Mahler wrote:
> > src/tests/master_validation_tests.cpp, lines 2589-2590
> > <https://reviews.apache.org/r/55381/diff/4/?file=1607721#file1607721line2589>
> >
> > Both empty would signify a change of roles since an empty `roles` indicates no roles rather than the default role of `"*"`.
> >
> > This test appears to be covering the case where they are both set to the same non-default value (so I'll update the comment), do you want to add another test that covers the `"*`" case or is the implementation agnostic to this case?
For framework upgrades to multi-role, I was interpreting changes of `role='*'` or `role=''` to `roles={}` as equivalent in follow-up patches, and I read your comment as that not being the case. Are the expected upgrade semantics `'*' -> {*}` and `'' -> {}`? If that is the case, is `{}` considered an error, or would such a framework receives offers for `*` as well? If `{}` is an error, we seem to be lacking validation for that as well.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review162351
-----------------------------------------------------------
On Jan. 18, 2017, 4:26 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 4:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review162351
-----------------------------------------------------------
Fix it, then Ship it!
Thanks, looks good as far as validation is concerned. Seems we'll need a more comprehensive upgrade test to ensure a framework with running tasks can do the upgrade and continue to function normally, as well as launch new tasks in the new model.
Just some minor comments below, I'll make the updates and get this committed.
src/tests/master_validation_tests.cpp (lines 2589 - 2590)
<https://reviews.apache.org/r/55381/#comment233698>
Both empty would signify a change of roles since an empty `roles` indicates no roles rather than the default role of `"*"`.
This test appears to be covering the case where they are both set to the same non-default value (so I'll update the comment), do you want to add another test that covers the `"*`" case or is the implementation agnostic to this case?
src/tests/master_validation_tests.cpp (line 2602)
<https://reviews.apache.org/r/55381/#comment233699>
How about using Duration here? e.g.
```
frameworkInfo.set_failover_timeout(Weeks(1).secs());
```
src/tests/master_validation_tests.cpp (lines 2618 - 2619)
<https://reviews.apache.org/r/55381/#comment233700>
How about a `driver.stop(failover=true); driver.join()` here to be more explicit?
src/tests/master_validation_tests.cpp (lines 2642 - 2643)
<https://reviews.apache.org/r/55381/#comment233701>
How about a stop and join here to be consistent with other tests that register a scheduler driver above?
- Benjamin Mahler
On Jan. 18, 2017, 3:26 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 3:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 4:26 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Rebased.
Bugs: MESOS-6900
https://issues.apache.org/jira/browse/MESOS-6900
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/master_validation_tests.cpp c092362152e1fe8a6b615c2eda171d852c1bbd86
Diff: https://reviews.apache.org/r/55381/diff/
Testing
-------
make check (OS X)
Thanks,
Benjamin Bannier
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Jay Guo <gu...@gmail.com>.
> On Jan. 16, 2017, 12:37 p.m., Jay Guo wrote:
> > src/tests/master_validation_tests.cpp, line 2559
> > <https://reviews.apache.org/r/55381/diff/3/?file=1603887#file1603887line2559>
> >
> > I think this test is not valid since we currently don't have logic to check framework upgrade yet.
> >
> > This test passes because `role` update is ignored with a warning generated here: https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491
> >
> > `roles` field is actually not checked against old `role`.
Just saw next patch in the review chain. Could you reverse the order since this test should actually goes after the framework upgrade patch.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161676
-----------------------------------------------------------
On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Jay Guo <gu...@gmail.com>.
> On Jan. 16, 2017, 12:37 p.m., Jay Guo wrote:
> > src/tests/master_validation_tests.cpp, line 2559
> > <https://reviews.apache.org/r/55381/diff/3/?file=1603887#file1603887line2559>
> >
> > I think this test is not valid since we currently don't have logic to check framework upgrade yet.
> >
> > This test passes because `role` update is ignored with a warning generated here: https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491
> >
> > `roles` field is actually not checked against old `role`.
>
> Jay Guo wrote:
> Just saw next patch in the review chain. Could you reverse the order since this test should actually goes after the framework upgrade patch.
>
> Benjamin Bannier wrote:
> I explicitly moved it to the start of the chain. That way we can make sure the next patch does not introduce regressions. What do you think.
I don't have strong opinion against this. Just feel a bit odd if we look at this test without next patches in the chain. Let me drop the issue.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161676
-----------------------------------------------------------
On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 16, 2017, 5:37 a.m., Jay Guo wrote:
> > src/tests/master_validation_tests.cpp, line 2559
> > <https://reviews.apache.org/r/55381/diff/3/?file=1603887#file1603887line2559>
> >
> > I think this test is not valid since we currently don't have logic to check framework upgrade yet.
> >
> > This test passes because `role` update is ignored with a warning generated here: https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491
> >
> > `roles` field is actually not checked against old `role`.
>
> Jay Guo wrote:
> Just saw next patch in the review chain. Could you reverse the order since this test should actually goes after the framework upgrade patch.
I explicitly moved it to the start of the chain. That way we can make sure the next patch does not introduce regressions. What do you think.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161676
-----------------------------------------------------------
On Jan. 12, 2017, 4:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 4:32 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161676
-----------------------------------------------------------
src/tests/master_validation_tests.cpp (line 2559)
<https://reviews.apache.org/r/55381/#comment232993>
I think this test is not valid since we currently don't have logic to check framework upgrade yet.
This test passes because `role` update is ignored with a warning generated here: https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2488-L2491
`roles` field is actually not checked against old `role`.
- Jay Guo
On Jan. 12, 2017, 11:32 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 4:32 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Addressed gyliu's comments.
Bugs: MESOS-6900
https://issues.apache.org/jira/browse/MESOS-6900
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
Diff: https://reviews.apache.org/r/55381/diff/
Testing
-------
make check (OS X)
Thanks,
Benjamin Bannier
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161371
-----------------------------------------------------------
src/tests/master_validation_tests.cpp (line 2559)
<https://reviews.apache.org/r/55381/#comment232613>
s/UpgradeToMultirole/UpgradeToMultiRole
- Guangya Liu
On \u4e00\u6708 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 11, 2017, 10:37 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/
-----------------------------------------------------------
(Updated Jan. 11, 2017, 11:37 a.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Rebased.
Bugs: MESOS-6900
https://issues.apache.org/jira/browse/MESOS-6900
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
Diff: https://reviews.apache.org/r/55381/diff/
Testing
-------
make check (OS X)
Thanks,
Benjamin Bannier
Re: Review Request 55381: Added test for framework upgrading to
multi-role capability.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55381/#review161102
-----------------------------------------------------------
Bad patch!
Reviews applied: [55381]
Failed command: ./support/apply-review.sh -n -r 55381
Error:
2017-01-10 19:23:04 URL:https://reviews.apache.org/r/55381/diff/raw/ [2169/2169] -> "55381.patch" [1]
error: patch failed: src/tests/master_validation_tests.cpp:2608
error: src/tests/master_validation_tests.cpp: patch does not apply
Full log: https://builds.apache.org/job/mesos-reviewbot/16669/console
- Mesos ReviewBot
On Jan. 10, 2017, 2:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55381/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 2:50 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6900
> https://issues.apache.org/jira/browse/MESOS-6900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55381/diff/
>
>
> Testing
> -------
>
> make check (OS X)
>
>
> Thanks,
>
> Benjamin Bannier
>
>