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

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

    either present or future, e.g. "it determines how often..."
    
    s/update/perform



include/mesos/master/allocator.hpp (lines 72 - 73)
<https://reviews.apache.org/r/37993/#comment156269>

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

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



include/mesos/master/allocator.hpp (lines 92 - 94)
<https://reviews.apache.org/r/37993/#comment156274>

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



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

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



include/mesos/master/allocator.hpp (lines 105 - 106)
<https://reviews.apache.org/r/37993/#comment156276>

    s/the removed framework's resources/they



include/mesos/master/allocator.hpp (lines 115 - 116)
<https://reviews.apache.org/r/37993/#comment156277>

    s/can only be/are



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

    s/Deactivate/Deactivates
    here and below



include/mesos/master/allocator.hpp (lines 124 - 125)
<https://reviews.apache.org/r/37993/#comment156279>

    s/will not be/are not



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

    s/Update/Updates



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

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

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

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



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

    s/will be/is



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

    s/old slave recovery/agent failover



include/mesos/master/allocator.hpp (lines 172 - 173)
<https://reviews.apache.org/r/37993/#comment156292>

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

    s/Update/Updates



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

    Activates



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

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

Reply via email to