> On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1757-1779
> > <https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1757>
> >
> >     checking validationError.isNone() in each if statement looks a bit 
> > weird. how about doing these in an else if instead?
> >     
> >     if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
> >     
> >     } else if (!roles.contains(frameworkInfo.role()) {
> >     
> >     } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
> >     
> >     } else {
> >     
> >     }
> >     
> >     
> >     i think the above is easier to read?
> 
> Ben Mahler wrote:
>     That's originally what I did, but it's harder to read that way. It looks 
> saner in your example because you used empty blocks. Here is what it would 
> actually look like:
>     
>     ```
>       // TODO(vinod): Add "!=" operator for FrameworkID.
>       if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
>         validationError = Error("Registering with 'id' already set");
>       } else if (!roles.contains(frameworkInfo.role())) {
>         // TODO(vinod): Deprecate this in favor of ACLs.
>         validationError = Error("Role '" + frameworkInfo.role() + "' is not" +
>                                 " present in the master's --roles");
>       } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
>         // TODO(vinod): Deprecate this in favor of authorization.
>         validationError = Error("User 'root' is not allowed to run frameworks"
>             " without --root_submissions set");
>       }
>     ```
>     
>     Also the it doesn't work so nicely for reregister because one of the 
> checks requires looping through framework ids.

It does look dense because of the comments but not that bad IMO. For example, 
you used the same pattern for checking authorization failure.

AFAIK we haven't used the pattern that you are proposing elsewhere and it reads 
weird to me. I would rather you just return with an error right away, even 
though it might be a bit verbose. What do you think?


- Vinod


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


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> -----------------------------------------------------------
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Reply via email to