Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

> On Nov. 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 413-418
> > 
> >
> > Rather than doing the math here  (which I believe we're missing 
> > corresponding entries for in `removeSlave()`?) why not test whether 
> > `slaves.size() >= expectedAgentCount()` ?
> > 
> > I think it is more clear, and less error prone.
> > 
> > Let's log when this condition is met?
> 
> Alexander Rukletsov wrote:
> Let me address your concerns one by one : )
> 
> 1. I don't think we need corresponding entries in `removeSlave()` because 
> we do not track what agents reregistered, but rather a total number. If an 
> agent reregistered and then got lost, this should not influence the 
> allocation pause.
> 2. For the reason described in 1., `slaves.size() >= 
> expectedAgentCount()` seems not a good fit, because it counts removed agents 
> in.
> 3. Logging is a good idea.

If we have just a number for recovered agents, we cannot distinguish between 
“old” agents from the registry and “new” ones joined after recovery. Because we 
do not persist enough information to base logical decisions on, any accounting 
algorithm here will be crude. Hence we decided to pick Joris' suggestion. Doing 
`slaves.size() >= expectedAgents` at least expresses the intention.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107607
---


On Nov. 30, 2015, 3:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 30, 2015, 3:32 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1cd8d16661568010901e74705375e7719cdfb8a0 
>   src/master/allocator/mesos/hierarchical.cpp 
> 31ed62efb5b1a2edb567f43d37559c5914e0665e 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Dec. 3, 2015, 8:07 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Changes
---

Update recovery algorithm


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Dec. 3, 2015, 9:10 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Changes
---

Skipped recovery if there are no agents.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review108925
---

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 3, 2015, 9:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Dec. 3, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1cd8d16661568010901e74705375e7719cdfb8a0 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8f65b72488505afd3677ecdeb9b821ea27af7e0 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review108615
---

Ship it!


Ship It!

- Qian Zhang


On Nov. 30, 2015, 11:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 30, 2015, 11:32 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1cd8d16661568010901e74705375e7719cdfb8a0 
>   src/master/allocator/mesos/hierarchical.cpp 
> 31ed62efb5b1a2edb567f43d37559c5914e0665e 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 30, 2015, 3:32 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Changes
---

Rebased.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
31ed62efb5b1a2edb567f43d37559c5914e0665e 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 24, 2015, 3:07 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Changes
---

Rephrased logs.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
  src/master/allocator/mesos/hierarchical.cpp 
aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Joris Van Remoortere]

> On Nov. 23, 2015, 2:40 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 171
> > 
> >
> > What about rename it to recoverQuota()?

We want to have just a single recover function, just like other modules in the 
master.
The fact that only quota is currently recovered in the hierarchical allocator 
is a detail, not a constraint. Does this make more sense?


- Joris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107539
---


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 23, 2015, 10:44 a.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
c65fe35198b846da2dc959dd467a21ff6edd30a9 
  src/master/allocator/mesos/hierarchical.cpp 
2765526047767cbd19d13c11ecfa6e90c505b3a7 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

> On Nov. 23, 2015, 2:40 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 171
> > 
> >
> > What about rename it to recoverQuota()?
> 
> Joris Van Remoortere wrote:
> We want to have just a single recover function, just like other modules 
> in the master.
> The fact that only quota is currently recovered in the hierarchical 
> allocator is a detail, not a constraint. Does this make more sense?

It's true that now we recover only quota, but I would prefer to keep 
`recover()` general. Other allocators (or we in the future) may use it for 
something different.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107539
---


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

> On Nov. 23, 2015, 9:22 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 192
> > 
> >
> > I think what we want to use should be master's flag: 
> > `slave_reregister_timeout`, so can we just pass it into allocator when 
> > master initializes allocator rather than defining a separate const here?

Not necessarily. For example, this value may be greater than 
`slave_reregister_timeout` with the idea that for an operator may add some 
agents instead of lost ones. It can also be smaller, which means we start 
allocation without waiting for agents to reregister.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107568
---


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

> On Nov. 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 177-178
> > 
> >
> > This first invariant is something we push on the user of the allocator 
> > API (i.e. don't call `addSlave()` until after `recover()` is called).
> > The second is an internal invariant we maintain between `initialize()` 
> > and `recover()`.
> > Can we clarify this with a comment, maybe even split them out in 
> > separate code blocks?
> > 
> > I know this seems picky, but we try not to use `CHECK`s for external 
> > invariants (like L177). We happen to do so currently in the allocator 
> > because it is so tightly integrated with the master. Ideally we can remove 
> > these external ones once we refactor; however, we would want to keep the 
> > internal one.
> > Does this make sense?

That's a good remark and it does make sense in general. However in this case, 
both are checks are API-related. The second one (L178) ensures a client does 
not call `setQuota()` until after `recover()` is called. Actually, similar is 
with `CHECK(initialized)`: we ensure `initialize()` is called before all other 
functions.


> On Nov. 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 191-193
> > 
> >
> > Is there a ticket for this? I think we'll want to do this for the MVP.
> > Thoughts?

I am a bit reluctant to expose these constants as master flags, because they 
are rather implementation specific. It would be nice to combine all allocator 
flags into one `JSON`? file, similar to how we do it for modules. This requires 
some work, hence I put a TODO without a ticket and am not convinced it's 
mandatory in the MVP.


> On Nov. 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 413-418
> > 
> >
> > Rather than doing the math here  (which I believe we're missing 
> > corresponding entries for in `removeSlave()`?) why not test whether 
> > `slaves.size() >= expectedAgentCount()` ?
> > 
> > I think it is more clear, and less error prone.
> > 
> > Let's log when this condition is met?

Let me address your concerns one by one : )

1. I don't think we need corresponding entries in `removeSlave()` because we do 
not track what agents reregistered, but rather a total number. If an agent 
reregistered and then got lost, this should not influence the allocation pause.
2. For the reason described in 1., `slaves.size() >= expectedAgentCount()` 
seems not a good fit, because it counts removed agents in.
3. Logging is a good idea.


> On Nov. 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 988-997
> > 
> >
> > Let's most definitely log this :-)
> > It should happen rarely, so it's not a big deal to log.
> > Considering the conversations we had with other developers, this is 
> > definitely something they will want to know when debugging ;-)

Could not agree more!


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107607
---


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

> On Nov. 23, 2015, 2:07 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 193
> > 
> >
> > Can we add some comments for why using 0.8 as default?

They should be exposed as parameters.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107535
---


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 23, 2015, 11:35 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
  src/master/allocator/mesos/hierarchical.cpp 
aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Guangya Liu]

> On 十一月 23, 2015, 2:40 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 171
> > 
> >
> > What about rename it to recoverQuota()?
> 
> Joris Van Remoortere wrote:
> We want to have just a single recover function, just like other modules 
> in the master.
> The fact that only quota is currently recovered in the hierarchical 
> allocator is a detail, not a constraint. Does this make more sense?
> 
> Alexander Rukletsov wrote:
> It's true that now we recover only quota, but I would prefer to keep 
> `recover()` general. Other allocators (or we in the future) may use it for 
> something different.

Fair enough. I also consider the recover case, it is useful to recover a 
specified number of slave hosts for allocator before allocating resources.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107539
---


On 十一月 23, 2015, 11:35 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated 十一月 23, 2015, 11:35 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
>   src/master/allocator/mesos/hierarchical.cpp 
> aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Guangya Liu]

> On 十一月 23, 2015, 5:54 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 191-193
> > 
> >
> > Is there a ticket for this? I think we'll want to do this for the MVP.
> > Thoughts?
> 
> Alexander Rukletsov wrote:
> I am a bit reluctant to expose these constants as master flags, because 
> they are rather implementation specific. It would be nice to combine all 
> allocator flags into one `JSON`? file, similar to how we do it for modules. 
> This requires some work, hence I put a TODO without a ticket and am not 
> convinced it's mandatory in the MVP.

There are some discussions planning to put all flags to a configuraiton file: 
http://search-hadoop.com/m/0Vlr6jduNa1HKP7v1/configuration+files=Configuration+file+


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107607
---


On 十一月 23, 2015, 11:35 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated 十一月 23, 2015, 11:35 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
>   src/master/allocator/mesos/hierarchical.cpp 
> aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107692
---



src/master/allocator/mesos/hierarchical.cpp (line 416)


There might be new host added when recovery, so what about renaming 
"reregistered" to "added"?

s/reregistered/added?



src/master/allocator/mesos/hierarchical.cpp (line 993)


Shall we unify the tone in log files? What about s/Pausing/Paused



src/master/allocator/mesos/hierarchical.cpp (line 1003)


s/Resuming/Resumed


- Guangya Liu


On 十一月 23, 2015, 11:35 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated 十一月 23, 2015, 11:35 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
>   src/master/allocator/mesos/hierarchical.cpp 
> aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 23, 2015, 4:11 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Changes
---

Rebased.


Bugs: MESOS-3981
https://issues.apache.org/jira/browse/MESOS-3981


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
c65fe35198b846da2dc959dd467a21ff6edd30a9 
  src/master/allocator/mesos/hierarchical.cpp 
2765526047767cbd19d13c11ecfa6e90c505b3a7 

Diff: https://reviews.apache.org/r/40332/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107607
---



src/master/allocator/mesos/hierarchical.hpp (line 196)


s/Methods/Functions/Helpers/
Methods is more of a java-ism :-)



src/master/allocator/mesos/hierarchical.cpp (lines 177 - 178)


This first invariant is something we push on the user of the allocator API 
(i.e. don't call `addSlave()` until after `recover()` is called).
The second is an internal invariant we maintain between `initialize()` and 
`recover()`.
Can we clarify this with a comment, maybe even split them out in separate 
code blocks?

I know this seems picky, but we try not to use `CHECK`s for external 
invariants (like L177). We happen to do so currently in the allocator because 
it is so tightly integrated with the master. Ideally we can remove these 
external ones once we refactor; however, we would want to keep the internal one.
Does this make sense?



src/master/allocator/mesos/hierarchical.cpp (line 184)


new line.



src/master/allocator/mesos/hierarchical.cpp (line 187)


s/after/after a/



src/master/allocator/mesos/hierarchical.cpp (lines 191 - 193)


Is there a ticket for this? I think we'll want to do this for the MVP.
Thoughts?



src/master/allocator/mesos/hierarchical.cpp (line 198)


s/Record a/Record the/



src/master/allocator/mesos/hierarchical.cpp (lines 199 - 200)


Let's drop the `this`, and rename the parameter with a `_`.



src/master/allocator/mesos/hierarchical.cpp (line 412)


Should we `CHECK(!paused || expectedAgentCount.isSome());` above this?



src/master/allocator/mesos/hierarchical.cpp (lines 413 - 418)


Rather than doing the math here  (which I believe we're missing 
corresponding entries for in `removeSlave()`?) why not test whether 
`slaves.size() >= expectedAgentCount()` ?

I think it is more clear, and less error prone.

Let's log when this condition is met?



src/master/allocator/mesos/hierarchical.cpp (lines 988 - 997)


Let's most definitely log this :-)
It should happen rarely, so it's not a big deal to log.
Considering the conversations we had with other developers, this is 
definitely something they will want to know when debugging ;-)



src/master/allocator/mesos/hierarchical.cpp (lines 994 - 997)


Does it make sense to `CHECK(paused)` here?
If not, why not? I think making these semantics as clear as possible is 
crucial for such a high impact change.



src/master/allocator/mesos/hierarchical.cpp (lines 1009 - 1010)


Let's log that we are skipping allocation due to a paused allocator.
Same for the keyed version of this function below.


- Joris Van Remoortere


On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Alexander Rukletsov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/
---

(Updated Nov. 23, 2015, 12:31 a.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Summary (updated)
-

Quota: Implemented recovery in hierarchical allocator.


Bugs: MESOS-3874
https://issues.apache.org/jira/browse/MESOS-3874


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
c65fe35198b846da2dc959dd467a21ff6edd30a9 
  src/master/allocator/mesos/hierarchical.cpp 
2765526047767cbd19d13c11ecfa6e90c505b3a7 

Diff: https://reviews.apache.org/r/40332/diff/


Testing (updated)
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107535
---



src/master/allocator/mesos/hierarchical.cpp (line 193)


Can we add some comments for why using 0.8 as default?



src/master/allocator/mesos/hierarchical.cpp (line 206)


The log message should also be updated.


- Guangya Liu


On 十一月 23, 2015, 12:31 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated 十一月 23, 2015, 12:31 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3874
> https://issues.apache.org/jira/browse/MESOS-3874
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107539
---



src/master/allocator/mesos/hierarchical.cpp (line 171)


What about rename it to recoverQuota()?


- Guangya Liu


On 十一月 23, 2015, 12:31 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated 十一月 23, 2015, 12:31 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3874
> https://issues.apache.org/jira/browse/MESOS-3874
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40332/#review107543
---


Patch looks great!

Reviews applied: [40332]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 23, 2015, 12:31 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 23, 2015, 12:31 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3874
> https://issues.apache.org/jira/browse/MESOS-3874
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c65fe35198b846da2dc959dd467a21ff6edd30a9 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2765526047767cbd19d13c11ecfa6e90c505b3a7 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>