This is an automatically generated e-mail. To reply, visit:

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)


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

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

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 

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

    Re-adding a framework will not call this.

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

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

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

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

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


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

    I think this doc is a better reference than the JIRA: 
    (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)


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

    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:

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

    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)

    Not necessary.

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


include/mesos/master/allocator.hpp (lines 238 - 242)

    These two comments pretty much say the same thing.  I suggest moving 
merging them.
    @param whitelist A set of agents that are allowed to contribute their 
resources to the resource pool.

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


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


include/mesos/master/allocator.hpp (lines 253 - 255)

    Not necessary.

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


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

    s/operations, it may/operations.  The allocator should/

include/mesos/master/allocator.hpp (lines 269 - 272)

    Not necessary.

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

    Updates unavailability for an agent.

include/mesos/master/allocator.hpp (lines 296 - 299)

    Not necessary.

include/mesos/master/allocator.hpp (lines 311 - 312)

    Not necessary.

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

    Be careful with your rebase :)

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


include/mesos/master/allocator.hpp (lines 340 - 344)

    Used to update the set of available resources for a specific agent.  This 
method is invoked to inform the allocator about allocated resources that have 
been refused or are no longer in use.

include/mesos/master/allocator.hpp (lines 345 - 351)

    Not necessary.

include/mesos/master/allocator.hpp (lines 362 - 363)

    Revives offers for a framework.  This is invoked when by a framework when 
it wishes to receive filtered resources or offers immediately.

include/mesos/master/allocator.hpp (lines 364 - 365)

    Not necessary.

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


- Joseph Wu

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

Reply via email to