Re: Review Request 37993: Add explanatory comments for Allocator interface

[Michael Park]

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

Ship it!


I've shipped this patch with the minor typos and style fixes outlined below.


include/mesos/master/allocator.hpp (lines 111 - 112)


Indent 4 spaces.



include/mesos/master/allocator.hpp (line 153)


`s/new/newly/`



include/mesos/master/allocator.hpp (line 229)


`s/a/the/`



include/mesos/master/allocator.hpp (line 231)


`s/invokved/invoked/`
`s/a whitelist flag/the --whitelist flag`



include/mesos/master/allocator.hpp (line 254)


`s/reservationa/reservation/`



include/mesos/master/allocator.hpp (line 333)


`s/when//`



include/mesos/master/allocator.hpp (line 342)


`s/for/to/`


- Michael Park


On Oct. 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 十月 12, 2015, 6:15 p.m., Michael Park wrote:
> > I've shipped this patch with the minor typos and style fixes outlined below.

Thanks Michael Park for the update ;-)


- Guangya


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


On 十月 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 十月 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Oct. 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Joseph Wu]

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

Ship it!


This is looking very good now!

Thanks for being patient with our comments :)


include/mesos/master/allocator.hpp (line 154)


Typo: s/desing/design/



include/mesos/master/allocator.hpp (line 340)


Missing a period.


- Joseph Wu


On Oct. 3, 2015, 9:24 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 3, 2015, 9:24 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Oct. 6, 2015, 12:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 6, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 十月 6, 2015, 12:26 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 十月 4, 2015, 4:24 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Addressed Joseph and Alex's comments. Thanks both for the detailed review!


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Oct. 4, 2015, 4:24 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Oct. 4, 2015, 4:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 25, 2015, 4:21 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Rebase


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > Looking almost perfect! Really appreciate that you diligently address all 
> > the comments I throw into you and don't give up.
> > 
> > While you're touching this file, I would say it makes sense to clean up 
> > everything we can in one patch. Here are some suggestions:
> > - Wrap all types and variables in comments in backticks;
> > - As I've mentioned in one of my previous reviews, let's use the same tense 
> > everywhere in the commens for consistency;
> > - Not a native speaker, but I think we should consolidate the use of 
> > articles when referencing allocator in comments. I think "an allocator" 
> > makes more sense since it is explicitly indicates that it's implementation 
> > dependent and not tied to a particular implementation;
> > - Again, not a native speaker, but let's refer to a framework as "which" or 
> > "that", not "who".

Thanks Alex for the review, I found that there are new interfaces added to 
allocator and will add comments for the new interface later. You can take a 
look the updated patch to see if there are anything need improvement. ;-)


- Guangya


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


On 九月 25, 2015, 5:22 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 25, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Joseph Wu]

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


Per AlexR, I'll review as a native speaker.

General note:
There are quite a few run-on sentences (i.e. using commas instead of periods).  
This is fairly common in chinese, but most english word processors will point 
this out automatically :P


include/mesos/master/allocator.hpp (line 45)


Add a newline after `/**` (Inconsistent style.)



include/mesos/master/allocator.hpp (line 51)


s/a/an/



include/mesos/master/allocator.hpp (lines 75 - 78)


Suggestion:

Any errors in initialization should fail fast and result in an `ABORT`.  
The master expects the allocator to be successfully initialized if this call 
returns.



include/mesos/master/allocator.hpp (lines 86 - 88)


The roles are actually checked by the master (see `Master::subscribe`).  
Instead, you could replace the second sentence with:

All frameworks that are added to the allocator will fall into one of these 
roles.



include/mesos/master/allocator.hpp (line 104)


Re-adding a framework will not call this.
See: 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L5370-L5371



include/mesos/master/allocator.hpp (lines 108 - 110)


Note that this field is non-zero when the master fails-over.  The 
frameworks report that they are using resources to the recovered master and 
that gets passed to the allocator.

Suggestion:

Resources used this framework.  The allocator should account for these 
resources when updating the allocation of this framework.



include/mesos/master/allocator.hpp (lines 130 - 131)


Suggestion:

Activates a framework in the Mesos cluster.  Offers are only sent to active 
frameworks.



include/mesos/master/allocator.hpp (lines 139 - 140)


Suggestion:

Deactivates a framework in the Mesos cluster.  Resource offers are not sent 
to deactivated frameworks.



include/mesos/master/allocator.hpp (line 150)


s/can/should/



include/mesos/master/allocator.hpp (line 152)


I think this doc is a better reference than the JIRA: 

https://cwiki.apache.org/confluence/display/MESOS/Design+doc:+Updating+Framework+Info

(If it's too long for one line, add ` // NOLINT` at the end.)



include/mesos/master/allocator.hpp (lines 154 - 156)


`@param` not really necessary.



include/mesos/master/allocator.hpp (line 168)


Remove "going".



include/mesos/master/allocator.hpp (line 169)


Period after "Detailed info of the agent".



include/mesos/master/allocator.hpp (line 171)


s/'total'/`total`/



include/mesos/master/allocator.hpp (lines 186 - 187)


Suggestion: 

Removes an agent from the Mesos cluster.  All resources belonging to this 
agent should be released by the allocator.



include/mesos/master/allocator.hpp (lines 188 - 189)


Not really necessary.



include/mesos/master/allocator.hpp (line 202)


Not really necessary.



include/mesos/master/allocator.hpp (lines 203 - 205)


Split the comment into two sentences:

s/,/./



include/mesos/master/allocator.hpp (lines 214 - 215)


Suggestion: 

Activates an agent.  This is invoked when an agent reregisters.  Offers are 
only sent for activated agents.



include/mesos/master/allocator.hpp (lines 216 - 217)


Not necessary.



include/mesos/master/allocator.hpp (lines 225 - 227)


The second sentence is incorrect:
* The allocator should treat all offers from the deactivated agent as 
rescinded.  (There is no separate call to the allocator to handle this).
* Resources aren't "recovered" when an agent deactivates (because the 
resources are lost).



include/mesos/master/allocator.hpp (lines 228 - 229)

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 25, 2015, 4:21 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 25, 2015, 4:21 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 54
> > 
> >
> > I think you killed that line in previous versions of the review, why do 
> > you want to bring it back?

removed.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 60-61
> > 
> >
> > Not yours, but let's wrap all types and variable names in backticks "`"

Not quite catch your point, can you please show more detail for what does this 
mean?


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 291
> > 
> >
> > s/time/interval

I was using "time interval" here, hope it is OK.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 270-271
> > 
> >
> > Again, I'm afraid the next engineer to update the set of offer 
> > operations will forget to update this list. Do you think it's valuable to 
> > keep this list here explicitly?

Agree,  I have removed the list here.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 151-152
> > 
> >
> > I'm a bit hesitant to put a reference to the built-in allocator here. 
> > The behaviour you describe is correct, but 
> > 1) It's how the built-in allocator works
> > 2) It's how the built-in allocator works now: when it the behaviour 
> > changes, my bet is that this comment won't be updated, hence it will become 
> > misleading.
> > 
> > What do you think?

Yes, I have removed this part.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 162
> > 
> >
> > s/new agent joins the Mesos cluster or in the case of a agent 
> > recovery./new agent joins the cluster or in case of agent recovery.
> > 
> > not a native speaker though

updated.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 183
> > 
> >
> > s/should/are

I was replacing "should be" to "is", hope it is ok.


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 193
> > 
> >
> > What do you mean by "new" here? New feature or new update?

new update, updated the comments here as: Updates the latest 


> On 九月 24, 2015, 4:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 247-250
> > 
> >
> > Suggestion:
> > 
> > A framework may request resources via this call. It is up to an 
> > allocator how to react to this request. For example, a request may be 
> > ignored, or may influence internal priorities an allocator may keep for 
> > frameworks.

Done.


- Guangya


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


On 九月 25, 2015, 4:21 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 25, 2015, 4:21 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 25, 2015, 5:22 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 25, 2015, 5:22 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 25, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Joseph Wu]

> On Sept. 24, 2015, 9:35 a.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 60-61
> > 
> >
> > Not yours, but let's wrap all types and variable names in backticks "`"
> 
> Guangya Liu wrote:
> Not quite catch your point, can you please show more detail for what does 
> this mean?

i.e. 
s/Try/`Try`/
s/Allocator*/`Allocator*`/


- Joseph


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


On Sept. 25, 2015, 10:22 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 25, 2015, 10:22 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 8100f14220599247a340c919a03f29755b5349d8 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Alexander Rukletsov]

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


Looking almost perfect! Really appreciate that you diligently address all the 
comments I throw into you and don't give up.

While you're touching this file, I would say it makes sense to clean up 
everything we can in one patch. Here are some suggestions:
- Wrap all types and variables in comments in backticks;
- As I've mentioned in one of my previous reviews, let's use the same tense 
everywhere in the commens for consistency;
- Not a native speaker, but I think we should consolidate the use of articles 
when referencing allocator in comments. I think "an allocator" makes more sense 
since it is explicitly indicates that it's implementation dependent and not 
tied to a particular implementation;
- Again, not a native speaker, but let's refer to a framework as "which" or 
"that", not "who".


include/mesos/master/allocator.hpp (lines 45 - 51)


While you're touching this file, let's doxygenify this one as well.



include/mesos/master/allocator.hpp (line 54)


I think you killed that line in previous versions of the review, why do you 
want to bring it back?



include/mesos/master/allocator.hpp (lines 60 - 61)


Not yours, but let's wrap all types and variable names in backticks "`"



include/mesos/master/allocator.hpp (lines 74 - 75)


Mind switching to Present Simple here for consistency? s/will be/is



include/mesos/master/allocator.hpp (line 75)


I'm not sure `initialize()` can fail: it returns nothing and we do not 
throw exceptions. I see two possibilities here:
- an allocator uses an exception, which is not caught in the master => 
master fails over;
- an allocator uses `CHECK*` macros for error situations => master fails 
over.

Could you please rephrase that in order not to mislead the reader?



include/mesos/master/allocator.hpp (line 78)


s/perform the allocation/perform the batch allocation/

An allocator may also perform allocation based on events (a framework is 
added and so on). However, it's up to the implementation.



include/mesos/master/allocator.hpp (lines 151 - 152)


I'm a bit hesitant to put a reference to the built-in allocator here. The 
behaviour you describe is correct, but 
1) It's how the built-in allocator works
2) It's how the built-in allocator works now: when it the behaviour 
changes, my bet is that this comment won't be updated, hence it will become 
misleading.

What do you think?



include/mesos/master/allocator.hpp (line 159)


s/a/an



include/mesos/master/allocator.hpp (line 161)


s/will be/is



include/mesos/master/allocator.hpp (line 162)


s/new agent joins the Mesos cluster or in the case of a agent recovery./new 
agent joins the cluster or in case of agent recovery.

not a native speaker though



include/mesos/master/allocator.hpp (line 180)


s/a/an

here and below.



include/mesos/master/allocator.hpp (line 183)


s/should/are



include/mesos/master/allocator.hpp (line 191)


s/a/an



include/mesos/master/allocator.hpp (line 193)


What do you mean by "new" here? New feature or new update?



include/mesos/master/allocator.hpp (line 208)


s/a/an

here and below



include/mesos/master/allocator.hpp (line 210)


s/will be/is
s/when re-register a agent/when an agent reregisters



include/mesos/master/allocator.hpp (line 213)


s/going to be/being



include/mesos/master/allocator.hpp (line 219)


s/a/an

here and below



include/mesos/master/allocator.hpp (lines 221 - 223)


How about this:

This is triggered if an agent disconnects from the master. Outstanding 
offers on a deactivated agent are removed and resources are recovered in a 
separate call.



include/mesos/master/allocator.hpp (line 232)

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Jian Qiu]

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



include/mesos/master/allocator.hpp (line 80)


needs adding explanation for @param inverseOfferCallback


- Jian Qiu


On Sept. 21, 2015, 7:28 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 21, 2015, 7:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 3fea47ffcc69531308068e2701502e481605b912 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 21, 2015, 7:28 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 21, 2015, 7:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 3fea47ffcc69531308068e2701502e481605b912 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 21, 2015, 7:28 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Summary (updated)
-

Add explanatory comments for Allocator interface


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs
-

  include/mesos/master/allocator.hpp 3fea47ffcc69531308068e2701502e481605b912 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Alexander Rukletsov]

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


I've commented on some issues. Several patterns I would like to ask you to 
correct everywhere, even where I didn't comment:

- same tense and mood;
- no shortcuts ("is not");
- avoid references to the built-in allocator implementation;
- focus on when the particular function is called and what the invariants are, 
say less about expectations, since implementation is up to the allocator.


include/mesos/master/allocator.hpp (line 67)


An allocator should not necessarily be a process. For actor-based 
allocators we have child interface `MesosAllocator`. I would suggest you 
rephrase that in a way, that successful initialization implies an allocator is 
ready to allocate, which includes setting up an actor process if necessary.



include/mesos/master/allocator.hpp (lines 70 - 71)


either present or future, e.g. "it determines how often..."

s/update/perform



include/mesos/master/allocator.hpp (lines 72 - 73)


Let's stick to one tense and mood, I would propose present indicative: "... 
the allocator uses to send..."

Allocator does not have the notion of offers (yet), its output is 
allocations.



include/mesos/master/allocator.hpp (lines 74 - 75)


Let's also document what happens if a framework uses a role unknown to the 
allocator.



include/mesos/master/allocator.hpp (lines 92 - 94)


Let's avoid conflating different usages of "used" here. Possible 
alternatives: leverage, rely on



include/mesos/master/allocator.hpp (line 104)


Let's avoid full forms in comments for interfaces: "it is", "should not" 
and so on.



include/mesos/master/allocator.hpp (lines 105 - 106)


s/the removed framework's resources/they



include/mesos/master/allocator.hpp (lines 115 - 116)


s/can only be/are



include/mesos/master/allocator.hpp (line 122)


s/Deactivate/Deactivates
here and below



include/mesos/master/allocator.hpp (lines 124 - 125)


s/will not be/are not



include/mesos/master/allocator.hpp (line 131)


s/Update/Updates



include/mesos/master/allocator.hpp (line 133)


Is it just capabilities that can be updated? I do remember several 
dicsussions and JIRA tickets around this topic. Do you think it makes sense to 
reference them here?



include/mesos/master/allocator.hpp (lines 137 - 139)


I would refrain from providing here examples about how real allocators 
work. The reason is that such comments are very difficult to maintain and they 
very often become misleading, which is even worse than non-existent.



include/mesos/master/allocator.hpp (line 150)


Let's use new terminology: agent instead of slave. Here and everywhere.



include/mesos/master/allocator.hpp (line 152)


s/will be/is



include/mesos/master/allocator.hpp (line 153)


s/old slave recovery/agent failover



include/mesos/master/allocator.hpp (lines 172 - 173)


I would phrase that in something like: "an allocator must consider all 
related resources unavailable and do not offer them to frameworks".



include/mesos/master/allocator.hpp (line 181)


s/Update/Updates



include/mesos/master/allocator.hpp (line 198)


Activates



include/mesos/master/allocator.hpp (line 200)


s/will be/is


- Alexander Rukletsov


On Sept. 8, 2015, 3:18 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 8, 2015, 3:18 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> 

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 5:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 7, 2015, 5:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 8, 2015, 3:18 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Update comments for whitelist, only hosts in white list can offer resources.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 3:18 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 8, 2015, 3:18 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 1:15 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 7, 2015, 1:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 7, 2015, 1:15 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


Changes
---

Update comments for whitelist


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 7, 2015, 5:10 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 2, 2015, 6:14 p.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, lines 92-93
> > 
> >
> > These two @param's don't add much, so you could exclude them.
> > 
> > I added comments like this in the past, which led to this email thread:
> > http://www.mail-archive.com/dev%40mesos.apache.org/msg32792.html

Done.


> On 九月 2, 2015, 6:14 p.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, line 111
> > 
> >
> > Again, this doesn't add much.  It's up to you if you want to keep it or 
> > not.
> > 
> > Same for all the similar @param's below (not marked).

All rmeoved if too simple


> On 九月 2, 2015, 6:14 p.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, line 143
> > 
> >
> > Again, try not to document the HierarchicalDRF ("the built-in 
> > allocator").
> > 
> > s/revocable/revocable resources/

I want to add some comments for built-in allocator so that the end user can 
take some reference for this, make snese? Thanks.


> On 九月 2, 2015, 6:14 p.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, lines 217-218
> > 
> >
> > Suggestion:
> > An agent may be deactivated if it is disconnnected from the master.

I was using slave here to keep consistent, will upload another patch to rename 
slave to agent, hope it is OK.


- Guangya


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


On 九月 2, 2015, 3:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 2, 2015, 3:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 3, 2015, 4:05 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 4:05 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 3, 2015, 4:05 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 90
> > 
> >
> > I think "register" is misleading. Allocator is notified that a new 
> > framework joins the cluster and is entitled to participate in resource 
> > sharing.

I was now using adds and re-adds instead in the new patch, comments?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 109
> > 
> >
> > Let's capiralize `Id` for clarity. Here and everywhere.

Done


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 139-142
> > 
> >
> > This it true for the built-in allocator, but not necessarily for *any* 
> > allocator. Could you please reword putting an accent on *under what 
> > cercumstances* the method is called and maybe what an expected behaviour 
> > may be?

Since the updateFramework is a new API and I can only think up of cases for 
revocable resources related with allocator, can we update this comments in 
future if there are more cases? Thaks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 171
> > 
> >
> > We try to switch to "agent" instead of "slave". Let's do it in the 
> > comments (here and everywhere). Also, let's have a note in the beginning of 
> > the doc saying "agent" is the new "slave".

I see that we already switched to AgentID in v1 API, but the V1 API was not 
finished. Is it possible that we keep slave here and fix this in another RR? Do 
you think that we need to file a new bug or epic to address this? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 213
> > 
> >
> > I think we remove only outstanding offers, right?

Alex, can you please show mode detail for what is outstanding offers? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 222
> > 
> >
> > I think "agent" is more clear than "host" here. You maybe refered to 
> > the fact that the whitelist consists of hostnames, but that's slightly 
> > different and should be documented in the whitelist class.

Here I was updating to "host" to "slave" instead and want to address the issue 
of "slave"->"agent" in another patch, make sense?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 226
> > 
> >
> > I don't think this is correct. AFAIK whitelist contains slaves that 
> > should not participate in the allocation, basically, they are deactivated.

I cite some code from allocator here, it seems that only hosts in whitelist can 
offer resources?

foreach (const SlaveID& slaveId, slaveIds) {
// Don't send offers for non-whitelisted and deactivated slaves.
if (!isWhitelisted(slaveId) || !slaves[slaveId].activated) {
  continue;
}


- Guangya


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


On 九月 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated 九月 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 2, 2015, 3:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 2, 2015, 3:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37993]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Alexander Rukletsov]

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



include/mesos/master/allocator.hpp (lines 62 - 64)


I think we can kill this for brevity.



include/mesos/master/allocator.hpp (line 68)


Period in the end, please!



include/mesos/master/allocator.hpp (line 70)


I think we tend to merge detailed description with the summary in case both 
are very short. Here and below.

Also, let's mention that calling `initialize()` supposes to start 
allocation process.



include/mesos/master/allocator.hpp (line 72)


Please a period at the end!



include/mesos/master/allocator.hpp (line 87)


I think we prefer 's' at the end of the verb in such cases: `Add*s* a 
framework`.



include/mesos/master/allocator.hpp (line 89)


I think "register" is misleading. Allocator is notified that a new 
framework joins the cluster and is entitled to participate in resource sharing.



include/mesos/master/allocator.hpp (lines 105 - 106)


It's an interface, it cannot guarantee that resoruces will be released. We 
should document an expectation or a contract here. It's up to an allocator what 
to do in this case, the built-in just removes the framework from the fair share 
pool AFAIK. Let's reword.



include/mesos/master/allocator.hpp (line 108)


Let's capiralize `Id` for clarity. Here and everywhere.



include/mesos/master/allocator.hpp (line 117)


s/activated/active



include/mesos/master/allocator.hpp (lines 138 - 141)


This it true for the built-in allocator, but not necessarily for *any* 
allocator. Could you please reword putting an accent on *under what 
cercumstances* the method is called and maybe what an expected behaviour may be?



include/mesos/master/allocator.hpp (lines 154 - 158)


The comment about static reservations is important, let's keep it in the 
description. Again, let's add some information on when it's called.



include/mesos/master/allocator.hpp (line 170)


We try to switch to "agent" instead of "slave". Let's do it in the comments 
(here and everywhere). Also, let's have a note in the beginning of the doc 
saying "agent" is the new "slave".



include/mesos/master/allocator.hpp (line 212)


I think we remove only outstanding offers, right?



include/mesos/master/allocator.hpp (line 213)


... and resources recovered in a separate call.



include/mesos/master/allocator.hpp (line 221)


I think "agent" is more clear than "host" here. You maybe refered to the 
fact that the whitelist consists of hostnames, but that's slightly different 
and should be documented in the whitelist class.



include/mesos/master/allocator.hpp (line 225)


I don't think this is correct. AFAIK whitelist contains slaves that should 
not participate in the allocation, basically, they are deactivated.



include/mesos/master/allocator.hpp (lines 235 - 237)


Let's say here that a framework may request resources, but it is up to 
allocator whether and how to satisfy this request.



include/mesos/master/allocator.hpp (lines 280 - 283)


Also, when framework is deactivated.


- Alexander Rukletsov


On Sept. 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> ---
> 
> (Updated Sept. 1, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.
> 
> 
> Bugs: MESOS-2224
> https://issues.apache.org/jira/browse/MESOS-2224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explanatory comments for Allocator interface
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
> 
> Diff: https://reviews.apache.org/r/37993/diff/
> 
> 
> Testing
> ---
> 
> 
> 

Re: Review Request 37993: Add explanatory comments for Allocator interface

[Guangya Liu]

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

(Updated 九月 1, 2015, 2:59 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, and Michael Park.


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


Repository: mesos


Description
---

Add explanatory comments for Allocator interface


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 

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


Testing
---


Thanks,

Guangya Liu