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