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

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

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 

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

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

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)

    here and below

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

    s/will not be/are not

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


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)


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


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