mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.
Date Fri, 03 Mar 2017 23:09:53 GMT


> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 2526 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650787#file1650787line2553>
> >
> >     At this point, you have based it on `info` instead of `source`, right? Maybe
that's a little clearer since we just adjusted `info`?

Not quite. This is subtle because `getRoles` looks at `capabilities`, and `info.capabilities`
is updated later on below.
I thought it's probably cleaner to just initalize off of `source` rather than setting `capabilities`
earlier and
have it become order-sensitive.


> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3335-3337 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line3347>
> >
> >     Should we use .at() for these three lines that intend const access into the
map?

Not const-access, but yeah, I agree. We should.


> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 5992-5993 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line6004>
> >
> >     Per our offline discussion, looks like this belongs in one of the previous reviews
that addresses the addition of the FrameworkInfo into the message? Seems like we need to send
it always now that it includes the info.

Followed up here: https://reviews.apache.org/r/57282/


> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 6085-6091 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line6097>
> >
> >     This could be simplified to:
> >     
> >     ```
> >     const set<string> removedRoles = oldRoles - newRoles;
> >     ```
> >     
> >     If we were to add the `-` operator to the existing operators in stout/set.hpp.

Yeah, but the operators there are defined in the global namespace, and doesn't work for `operator-`
because of collisions.


- Michael


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


On March 3, 2017, 1:47 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
> 
> (Updated March 3, 2017, 1:47 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
>     https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
> 
> 
> Diff: https://reviews.apache.org/r/57110/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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