> On Oct. 17, 2014, 12:29 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 2358-2361 > > <https://reviews.apache.org/r/26817/diff/1/?file=723143#file723143line2358> > > > > We should only proceed with authorization if validations succeeded. > > Otherwise there is no point. > > > > Also, it is a bit weird that we do all validations for all tasks and > > then do all authorizations for all tasks. I wonder if we can solve the > > original problem (not able to distinguish between validation failure vs > > authorization failure) differently, e.g., by returning ValidationError > > instead of Error, where the former contains more context. > > > > struct ValidationError : Error > > { > > enum Reason; > > } > > > > Ofcourse this assumes, the "Reason" patch lands first. > > > > Thoughts?
I thought the same, and originally started down the path of doing both, but doing both asynchronously was tricky. If validation doesn't need to be a future, then we can just do the validations synchronously and then do the authentication, but returning the right thing would, indeed, need to include a Reason. Landing the other patch first is hard because we can't differentiate between the authentication/validation cases without this patch :) > On Oct. 17, 2014, 12:29 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 2586-2589 > > <https://reviews.apache.org/r/26817/diff/1/?file=723143#file723143line2586> > > > > The caller shouldn't call this method if authorization is disabled. > > Move this logic to the caller. if not, then the logic gets more complex as we can't assume that authorizations and validations are vectors of the same size. This may change if validations aren't asynchronous and we can return a reason directly. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26817/#review57190 ----------------------------------------------------------- On Oct. 17, 2014, 1:09 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26817/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 1:09 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > split authorization from task validation. allows > https://reviews.apache.org/r/26382/ to differentiate between validation > issues and task authorization issues. > > also replaced some > > with >> and changed the order of methods in the cpp to > match the continuation order. > > > Diffs > ----- > > src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 > src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699 > > Diff: https://reviews.apache.org/r/26817/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
