mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joseph Wu" <jos...@mesosphere.io>
Subject Re: Review Request 37993: Add explanatory comments for Allocator interface
Date Fri, 25 Sep 2015 19:21:11 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/37993/#comment157847>

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



include/mesos/master/allocator.hpp (line 51)
<https://reviews.apache.org/r/37993/#comment157848>

    s/a/an/



include/mesos/master/allocator.hpp (lines 75 - 78)
<https://reviews.apache.org/r/37993/#comment157854>

    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)
<https://reviews.apache.org/r/37993/#comment157856>

    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)
<https://reviews.apache.org/r/37993/#comment157858>

    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)
<https://reviews.apache.org/r/37993/#comment157861>

    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)
<https://reviews.apache.org/r/37993/#comment157862>

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



include/mesos/master/allocator.hpp (lines 139 - 140)
<https://reviews.apache.org/r/37993/#comment157863>

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



include/mesos/master/allocator.hpp (line 150)
<https://reviews.apache.org/r/37993/#comment157866>

    s/can/should/



include/mesos/master/allocator.hpp (line 152)
<https://reviews.apache.org/r/37993/#comment157865>

    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)
<https://reviews.apache.org/r/37993/#comment157867>

    `@param` not really necessary.



include/mesos/master/allocator.hpp (line 168)
<https://reviews.apache.org/r/37993/#comment157869>

    Remove "going".



include/mesos/master/allocator.hpp (line 169)
<https://reviews.apache.org/r/37993/#comment157870>

    Period after "Detailed info of the agent".



include/mesos/master/allocator.hpp (line 171)
<https://reviews.apache.org/r/37993/#comment157872>

    s/'total'/`total`/



include/mesos/master/allocator.hpp (lines 186 - 187)
<https://reviews.apache.org/r/37993/#comment157875>

    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)
<https://reviews.apache.org/r/37993/#comment157874>

    Not really necessary.



include/mesos/master/allocator.hpp (line 202)
<https://reviews.apache.org/r/37993/#comment157876>

    Not really necessary.



include/mesos/master/allocator.hpp (lines 203 - 205)
<https://reviews.apache.org/r/37993/#comment157877>

    Split the comment into two sentences:
    
    s/,/./



include/mesos/master/allocator.hpp (lines 214 - 215)
<https://reviews.apache.org/r/37993/#comment157878>

    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)
<https://reviews.apache.org/r/37993/#comment157879>

    Not necessary.



include/mesos/master/allocator.hpp (lines 225 - 227)
<https://reviews.apache.org/r/37993/#comment157887>

    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)
<https://reviews.apache.org/r/37993/#comment157881>

    Not necessary.



include/mesos/master/allocator.hpp (line 237)
<https://reviews.apache.org/r/37993/#comment157889>

    s/It/This/



include/mesos/master/allocator.hpp (lines 238 - 242)
<https://reviews.apache.org/r/37993/#comment157891>

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



include/mesos/master/allocator.hpp (line 250)
<https://reviews.apache.org/r/37993/#comment157892>

    s/an/the/



include/mesos/master/allocator.hpp (line 252)
<https://reviews.apache.org/r/37993/#comment157893>

    s/an/the/



include/mesos/master/allocator.hpp (lines 253 - 255)
<https://reviews.apache.org/r/37993/#comment157894>

    Not necessary.



include/mesos/master/allocator.hpp (line 265)
<https://reviews.apache.org/r/37993/#comment157896>

    s/An/The/



include/mesos/master/allocator.hpp (line 266)
<https://reviews.apache.org/r/37993/#comment157897>

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



include/mesos/master/allocator.hpp (lines 269 - 272)
<https://reviews.apache.org/r/37993/#comment157899>

    Not necessary.



include/mesos/master/allocator.hpp (line 290)
<https://reviews.apache.org/r/37993/#comment157901>

    Suggestion:
    
    Updates unavailability for an agent.



include/mesos/master/allocator.hpp (lines 296 - 299)
<https://reviews.apache.org/r/37993/#comment157900>

    Not necessary.



include/mesos/master/allocator.hpp (lines 311 - 312)
<https://reviews.apache.org/r/37993/#comment157903>

    Not necessary.



include/mesos/master/allocator.hpp (line 332)
<https://reviews.apache.org/r/37993/#comment157904>

    Be careful with your rebase :)



include/mesos/master/allocator.hpp (line 338)
<https://reviews.apache.org/r/37993/#comment157906>

    s/Recover/Recovers/



include/mesos/master/allocator.hpp (lines 340 - 344)
<https://reviews.apache.org/r/37993/#comment157910>

    Suggestion:
    
    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)
<https://reviews.apache.org/r/37993/#comment157907>

    Not necessary.



include/mesos/master/allocator.hpp (lines 362 - 363)
<https://reviews.apache.org/r/37993/#comment157913>

    Suggestion:
    
    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)
<https://reviews.apache.org/r/37993/#comment157914>

    Not necessary.



include/mesos/master/allocator.hpp (line 373)
<https://reviews.apache.org/r/37993/#comment157915>

    s/resources/offers/
    s/for/to/


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message