-----------------------------------------------------------
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
>
>
|