mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 37993: Add explanatory comments for Allocator interface
Date Thu, 24 Sep 2015 16:35:04 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37993/#review100393
-----------------------------------------------------------


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

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



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

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

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



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

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



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

    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 over.
    
    Could you please rephrase that in order not to mislead the reader?



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

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

    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 misleading.
    
    What do you think?



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

    s/a/an



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

    s/will be/is



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

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

    s/a/an
    
    here and below.



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

    s/should/are



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

    s/a/an



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

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



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

    s/a/an
    
    here and below



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

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



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

    s/going to be/being



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

    s/a/an
    
    here and below



include/mesos/master/allocator.hpp (lines 221 - 223)
<https://reviews.apache.org/r/37993/#comment157609>

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

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



include/mesos/master/allocator.hpp (lines 234 - 237)
<https://reviews.apache.org/r/37993/#comment157611>

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

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

    Suggestion: Requests resources for a framework.



include/mesos/master/allocator.hpp (lines 247 - 250)
<https://reviews.apache.org/r/37993/#comment157616>

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

    s/Update/Updates



include/mesos/master/allocator.hpp (lines 260 - 265)
<https://reviews.apache.org/r/37993/#comment157618>

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

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



include/mesos/master/allocator.hpp (lines 270 - 271)
<https://reviews.apache.org/r/37993/#comment157623>

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

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



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

    s/time/interval



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

    Unfinished sentence?



include/mesos/master/allocator.hpp (lines 355 - 361)
<https://reviews.apache.org/r/37993/#comment157633>

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

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


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