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

Looking almost perfect! Really appreciate that you diligently address all the 
comments I throw into you and don't give up.

While you're touching this file, I would say it makes sense to clean up 
everything we can in one patch. Here are some suggestions:
- Wrap all types and variables in comments in backticks;
- As I've mentioned in one of my previous reviews, let's use the same tense 
everywhere in the commens for consistency;
- Not a native speaker, but I think we should consolidate the use of articles 
when referencing allocator in comments. I think "an allocator" makes more sense 
since it is explicitly indicates that it's implementation dependent and not 
tied to a particular implementation;
- Again, not a native speaker, but let's refer to a framework as "which" or 
"that", not "who".

include/mesos/master/allocator.hpp (lines 45 - 51)

    While you're touching this file, let's doxygenify this one as well.

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

    I think you killed that line in previous versions of the review, why do you 
want to bring it back?

include/mesos/master/allocator.hpp (lines 60 - 61)

    Not yours, but let's wrap all types and variable names in backticks "`"

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

    Mind switching to Present Simple here for consistency? s/will be/is

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

    I'm not sure `initialize()` can fail: it returns nothing and we do not 
throw exceptions. I see two possibilities here:
    - an allocator uses an exception, which is not caught in the master => 
master fails over;
    - an allocator uses `CHECK*` macros for error situations => master fails 
    Could you please rephrase that in order not to mislead the reader?

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

    s/perform the allocation/perform the batch allocation/
    An allocator may also perform allocation based on events (a framework is 
added and so on). However, it's up to the implementation.

include/mesos/master/allocator.hpp (lines 151 - 152)

    I'm a bit hesitant to put a reference to the built-in allocator here. The 
behaviour you describe is correct, but 
    1) It's how the built-in allocator works
    2) It's how the built-in allocator works now: when it the behaviour 
changes, my bet is that this comment won't be updated, hence it will become 
    What do you think?

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


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

    s/will be/is

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

    s/new agent joins the Mesos cluster or in the case of a agent recovery./new 
agent joins the cluster or in case of agent recovery.
    not a native speaker though

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

    here and below.

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


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


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

    What do you mean by "new" here? New feature or new update?

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

    here and below

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

    s/will be/is
    s/when re-register a agent/when an agent reregisters

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

    s/going to be/being

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

    here and below

include/mesos/master/allocator.hpp (lines 221 - 223)

    How about this:
    This is triggered if an agent disconnects from the master. Outstanding 
offers on a deactivated agent are removed and resources are recovered in a 
separate call.

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

    How about this one, should be more readable:
    Updates a list of trusted agents

include/mesos/master/allocator.hpp (lines 234 - 237)

    How about this:
    It is invokved when the master starts up with a whitelist flag. In this 
case an allocator must allocate resources only to the hosts in the whitelist.

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

    A whitelist of agents that are allowed to contribute their resources to the 
allocation pool.
    (agents do not offer resources to frameworks directly, let's avoid 
misleading comments)

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

    Suggestion: Requests resources for a framework.

include/mesos/master/allocator.hpp (lines 247 - 250)

    A framework may request resources via this call. It is up to an allocator 
how to react to this request. For example, a request may be ignored, or may 
influence internal priorities an allocator may keep for frameworks.

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


include/mesos/master/allocator.hpp (lines 260 - 265)

    Updates allocation by applying offer operations.
    This call is mainly intended to support persistence-related features 
(dynamic reservationa and persistent volumes). An allocator may react 
differently for certain offer operations, it may use this call to update 
bookkeeping information related to the framework.

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

    s/who has just got resources./which reacted to an offer.

include/mesos/master/allocator.hpp (lines 270 - 271)

    Again, I'm afraid the next engineer to update the set of offer operations 
will forget to update this list. Do you think it's valuable to keep this list 
here explicitly?

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

    Let's kill this for now, it doesn't really add extra information.

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


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

    Unfinished sentence?

include/mesos/master/allocator.hpp (lines 355 - 361)

    Revives offers for a framework.
    It is invoked when a framework that has filtered resources before wants to 
revive offers for those resources.
    @param frameworkId ID of the framework which wants to revive offers.

include/mesos/master/allocator.hpp (lines 366 - 369)

    It's resolved, isn't it?

- Alexander Rukletsov

On Sept. 21, 2015, 7:28 a.m., Guangya Liu wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> -----------------------------------------------------------
> (Updated Sept. 21, 2015, 7:28 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 3fea47ffcc69531308068e2701502e481605b912 
> Diff: https://reviews.apache.org/r/37993/diff/
> Testing
> -------
> Thanks,
> Guangya Liu

Reply via email to