> On June 6, 2014, 8:53 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 664 > > <https://reviews.apache.org/r/22284/diff/1/?file=604519#file604519line664> > > > > Same comment as previous review and yan's review, how about having this > > return a validation result? > > > > i.e. > > > > // Returns the validation result. > > Future<Option<Error>> validate(...);
done > On June 6, 2014, 8:53 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1052-1064 > > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1052> > > > > How much does this slow down the tests? > > > > One suggestion: what if we add a backoff (including initial jitter) > > akin to what's done in the slave? That way, we can scale better to a large > > number of frameworks and we don't have to have this hack in the master. Of > > course, we would want to make the backoff for framework registration > > smaller than what's done in the slaves. > > > > If the slowdown is not so bad, we could remove this re-queueing, leave > > a ticket and TODO in sched.cpp and follow up after this change. Each time I > > look at this, it's difficult to understand the invariants that make the > > re-queuing correct so my brain would appreciate it :) > > > > It means that we could move this case into 'validate' as an invalid > > case, which makes the code simpler. It can slow them down up to a second and if there are tests that pause the clock during registration it will break them. Jitter is a good idea, but it is still a race. I would like to punt on this for now, and circle back later since this is not new and we already have TODO/ticket to track. > On June 6, 2014, 8:53 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1097-1111 > > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1097> > > > > Curious if these checks here and in _reregisterFramework are necessary > > for correctness or just good to have? Not saying that we should remove > > them, but if necessary for correctness might be good to spell out why. it is only important in _reregisterFramework() when another framework is authenticated. in all other cases, it is just to do the right thing. updated the comment. > On June 6, 2014, 8:53 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1099 > > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1099> > > > > How about "in the interim" instead of "before we are here", seems to > > more precisely describe the situation? if we say "in the interim" we have to say in the interim of "what". "before we are here" seems appropriate. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22284/#review44959 ----------------------------------------------------------- On June 9, 2014, 10:09 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22284/ > ----------------------------------------------------------- > > (Updated June 9, 2014, 10:09 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1307 > https://issues.apache.org/jira/browse/MESOS-1307 > > > Repository: mesos-git > > > Description > ------- > > Added authorization for roles during framework (re-)registration. > > > Diffs > ----- > > src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149 > src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3 > src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f > src/tests/master_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/22284/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
