You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/04/24 23:18:44 UTC
Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
Bugs: MESOS-7323
https://issues.apache.org/jira/browse/MESOS-7323
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7
src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9
src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
Diff: https://reviews.apache.org/r/58689/diff/1/
Testing
-------
`make check` and added a test.
Thanks,
Michael Park
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review173149
-----------------------------------------------------------
Bad review!
Reviews applied: [58689, 58688, 58687, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 56805, 57165, 57164]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On April 26, 2017, 10:47 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> MESOS-7323: Made `addSlave` conditionally activate the framework.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/2/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` not activate any
frameworks.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review173247
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On April 27, 2017, 2:54 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 27, 2017, 2:54 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> MESOS-7323: Made `addSlave` not activate any frameworks.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/3/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` not activate any
frameworks.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/
-----------------------------------------------------------
(Updated April 27, 2017, 8:13 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
Changes
-------
Moved the test from `upgrade_tests.cpp` to `master_tests.cpp`.
Bugs: MESOS-7323
https://issues.apache.org/jira/browse/MESOS-7323
Repository: mesos
Description
-------
MESOS-7323: Made `addSlave` not activate any frameworks.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
src/tests/master_tests.cpp d1828eb42e0aedc9330c3786bbd9bb63aa42a64e
src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
Diff: https://reviews.apache.org/r/58689/diff/4/
Changes: https://reviews.apache.org/r/58689/diff/3-4/
Testing
-------
`make check` and added a test.
Thanks,
Michael Park
Re: Review Request 58689: MESOS-7323: Made `addSlave` not activate any
frameworks.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/
-----------------------------------------------------------
(Updated April 26, 2017, 7:54 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
Changes
-------
Addressed the rest of bmahler's comments regarding the test.
Summary (updated)
-----------------
MESOS-7323: Made `addSlave` not activate any frameworks.
Bugs: MESOS-7323
https://issues.apache.org/jira/browse/MESOS-7323
Repository: mesos
Description (updated)
-------
MESOS-7323: Made `addSlave` not activate any frameworks.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
Diff: https://reviews.apache.org/r/58689/diff/3/
Changes: https://reviews.apache.org/r/58689/diff/2-3/
Testing
-------
`make check` and added a test.
Thanks,
Michael Park
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/
-----------------------------------------------------------
(Updated April 26, 2017, 3:47 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
Changes
-------
Addressed a part of bmahler's comments.
Bugs: MESOS-7323
https://issues.apache.org/jira/browse/MESOS-7323
Repository: mesos
Description (updated)
-------
MESOS-7323: Made `addSlave` conditionally activate the framework.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
Diff: https://reviews.apache.org/r/58689/diff/2/
Changes: https://reviews.apache.org/r/58689/diff/1-2/
Testing
-------
`make check` and added a test.
Thanks,
Michael Park
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172888
-----------------------------------------------------------
Bad review!
Reviews applied: [58689, 58688, 58687, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 56805, 57165, 57164]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On April 24, 2017, 11:18 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 24, 2017, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7
> src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/1/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Michael Park <mp...@apache.org>.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 379 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698680#file1698680line379>
> >
> > Hm.. I was looking for a `framework.active = true` in activateFramework but seems this was missed?
As we discussed, turns out I didn't need the `active` member. Removed.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 483-485 (original), 487-493 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698680#file1698680line487>
> >
> > Per our conversation, this doesn't seem necessary (intuitively addSlave shouldn't induce an activation of the framework within a role).
> >
> > I guess we can just remove the activation code here and that makes it a 2 line fix? No need to store the 'active' state anymore either?
Yep! Thanks for this.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172968
-----------------------------------------------------------
On April 24, 2017, 4:18 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 24, 2017, 4:18 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7
> src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/1/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` not activate any
frameworks.
Posted by Benjamin Mahler <bm...@apache.org>.
> On April 26, 2017, 12:59 a.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 537-539 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line537>
> >
> > This comment seems to be inconsistent with what the test is doing?
> >
> > Also, this doesn't look like an upgrade test, should we make it a master test?
>
> Michael Park wrote:
> Updated the comment to more accurately reflect the test.
>
> I was considering this to be a kind of an upgrade test, in the sense that
> we're upgrading the framework from one config to another.
>
> I'm not sure if we have some clear criteria for what qualifies as an upgrade test.
>
> But I'm also fine with this living in master test.
Well, my intention was described at the top of the file:
```
// This file contains upgrade integration tests. Note that tests
// in this file can only "spoof" an old version of a component,
// since these run against the current version of the repository.
// The long term plan for integration tests is to checkout
// different releases of the repository and run components from
// different releases against each other.
```
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172968
-----------------------------------------------------------
On April 27, 2017, 2:54 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 27, 2017, 2:54 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> MESOS-7323: Made `addSlave` not activate any frameworks.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/3/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` not activate any
frameworks.
Posted by Michael Park <mp...@apache.org>.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 537-539 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line537>
> >
> > This comment seems to be inconsistent with what the test is doing?
> >
> > Also, this doesn't look like an upgrade test, should we make it a master test?
>
> Michael Park wrote:
> Updated the comment to more accurately reflect the test.
>
> I was considering this to be a kind of an upgrade test, in the sense that
> we're upgrading the framework from one config to another.
>
> I'm not sure if we have some clear criteria for what qualifies as an upgrade test.
>
> But I'm also fine with this living in master test.
>
> Benjamin Mahler wrote:
> Well, my intention was described at the top of the file:
>
> ```
> // This file contains upgrade integration tests. Note that tests
> // in this file can only "spoof" an old version of a component,
> // since these run against the current version of the repository.
> // The long term plan for integration tests is to checkout
> // different releases of the repository and run components from
> // different releases against each other.
> ```
Ah, sorry about that. Moved to `master_tests.cpp`
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172968
-----------------------------------------------------------
On April 26, 2017, 7:54 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> MESOS-7323: Made `addSlave` not activate any frameworks.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/3/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Michael Park <mp...@apache.org>.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 537-539 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line537>
> >
> > This comment seems to be inconsistent with what the test is doing?
> >
> > Also, this doesn't look like an upgrade test, should we make it a master test?
Updated the comment to more accurately reflect the test.
I was considering this to be a kind of an upgrade test, in the sense that
we're upgrading the framework from one config to another.
I'm not sure if we have some clear criteria for what qualifies as an upgrade test.
But I'm also fine with this living in master test.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 612 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line612>
> >
> > Hm.. not obvious to me why this was needed, can you add a comment?
I don't think this is necessary. Removed.
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 657 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line657>
> >
> > Do you want to just pause the clock for the whole test to avoid relying on timers?
Done!
> On April 25, 2017, 5:59 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 682-683 (patched)
> > <https://reviews.apache.org/r/58689/diff/1/?file=1698681#file1698681line682>
> >
> > ... and the master will track this role under the framework, but the framework should not receive offers for this role?
> >
> > Should we test for that?
Added queries to the master to test that the framework is re-tracked.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172968
-----------------------------------------------------------
On April 26, 2017, 3:47 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 3:47 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> MESOS-7323: Made `addSlave` conditionally activate the framework.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 984a0a4e2671ee7bb2a3515849342f49f2c4e3aa
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/2/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 58689: MESOS-7323: Made `addSlave` conditionally
activate the framework.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58689/#review172968
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp
Lines 379 (patched)
<https://reviews.apache.org/r/58689/#comment246073>
Hm.. I was looking for a `framework.active = true` in activateFramework but seems this was missed?
src/master/allocator/mesos/hierarchical.cpp
Lines 483-485 (original), 487-493 (patched)
<https://reviews.apache.org/r/58689/#comment246078>
Per our conversation, this doesn't seem necessary (intuitively addSlave shouldn't induce an activation of the framework within a role).
I guess we can just remove the activation code here and that makes it a 2 line fix? No need to store the 'active' state anymore either?
src/tests/upgrade_tests.cpp
Lines 537-539 (patched)
<https://reviews.apache.org/r/58689/#comment246069>
This comment seems to be inconsistent with what the test is doing?
Also, this doesn't look like an upgrade test, should we make it a master test?
src/tests/upgrade_tests.cpp
Lines 612 (patched)
<https://reviews.apache.org/r/58689/#comment246070>
Hm.. not obvious to me why this was needed, can you add a comment?
src/tests/upgrade_tests.cpp
Lines 657 (patched)
<https://reviews.apache.org/r/58689/#comment246071>
Do you want to just pause the clock for the whole test to avoid relying on timers?
src/tests/upgrade_tests.cpp
Lines 682-683 (patched)
<https://reviews.apache.org/r/58689/#comment246072>
... and the master will track this role under the framework, but the framework should not receive offers for this role?
Should we test for that?
- Benjamin Mahler
On April 24, 2017, 11:18 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58689/
> -----------------------------------------------------------
>
> (Updated April 24, 2017, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Neil Conway.
>
>
> Bugs: MESOS-7323
> https://issues.apache.org/jira/browse/MESOS-7323
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7
> src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9
> src/tests/upgrade_tests.cpp b5a28b5161d896dff250b8ad012c8aac91c3f861
>
>
> Diff: https://reviews.apache.org/r/58689/diff/1/
>
>
> Testing
> -------
>
> `make check` and added a test.
>
>
> Thanks,
>
> Michael Park
>
>