mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Guangya Liu" <gyliu...@gmail.com>
Subject Re: Review Request 37993: Add explanatory comments for Allocator interface
Date Wed, 02 Sep 2015 03:04:08 GMT


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 90
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line90>
> >
> >     I think "register" is misleading. Allocator is notified that a new framework
joins the cluster and is entitled to participate in resource sharing.

I was now using adds and re-adds instead in the new patch, comments?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 109
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line109>
> >
> >     Let's capiralize `Id` for clarity. Here and everywhere.

Done


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, lines 139-142
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line139>
> >
> >     This it true for the built-in allocator, but not necessarily for *any* allocator.
Could you please reword putting an accent on *under what cercumstances* the method is called
and maybe what an expected behaviour may be?

Since the updateFramework is a new API and I can only think up of cases for revocable resources
related with allocator, can we update this comments in future if there are more cases? Thaks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 171
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line171>
> >
> >     We try to switch to "agent" instead of "slave". Let's do it in the comments
(here and everywhere). Also, let's have a note in the beginning of the doc saying "agent"
is the new "slave".

I see that we already switched to AgentID in v1 API, but the V1 API was not finished. Is it
possible that we keep slave here and fix this in another RR? Do you think that we need to
file a new bug or epic to address this? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 213
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line213>
> >
> >     I think we remove only outstanding offers, right?

Alex, can you please show mode detail for what is outstanding offers? Thanks.


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 222
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line222>
> >
> >     I think "agent" is more clear than "host" here. You maybe refered to the fact
that the whitelist consists of hostnames, but that's slightly different and should be documented
in the whitelist class.

Here I was updating to "host" to "slave" instead and want to address the issue of "slave"->"agent"
in another patch, make sense?


> On 九月 1, 2015, 4:56 p.m., Alexander Rukletsov wrote:
> > include/mesos/master/allocator.hpp, line 226
> > <https://reviews.apache.org/r/37993/diff/2/?file=1061146#file1061146line226>
> >
> >     I don't think this is correct. AFAIK whitelist contains slaves that should not
participate in the allocation, basically, they are deactivated.

I cite some code from allocator here, it seems that only hosts in whitelist can offer resources?

foreach (const SlaveID& slaveId, slaveIds) {
    // Don't send offers for non-whitelisted and deactivated slaves.
    if (!isWhitelisted(slaveId) || !slaves[slaveId].activated) {
      continue;
    }


- Guangya


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


On 九月 1, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37993/
> -----------------------------------------------------------
> 
> (Updated 九月 1, 2015, 2:59 p.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