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, 17 Sep 2015 16:52:45 GMT

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


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