mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 71864: Moved creating authorization Object out of `Master::authorize*reserve`.
Date Thu, 02 Jan 2020 20:58:50 GMT


> On Dec. 4, 2019, 8:27 p.m., Benjamin Mahler wrote:
> > src/master/authorization.cpp
> > Lines 111 (patched)
> > <https://reviews.apache.org/r/71864/diff/1/?file=2181790#file2181790line111>
> >
> >     This comment isn't helpful, can you describe what this does? 
> >     
> >     It seems a lot easier to understand this function if this just returned action
objects instead of pushing into an existing vector (and push being in the name?). Further,
wouldn't this be even simpler if it just took a singular resource and the caller loops?
> >     
> >     Ditto for the one below.
> >     
> >     If the additonal push behavior is helpful, maybe just lamdba them in reserve
since that's where the code uses them several times within complex logic?

Thanks! Tried to improve the comments.

Unfortunately, looping in a caller does not look reasonable: note that some resources should
be skipped.
Given that both helpers might be called several times in `reserve()`, the idea to copy returned
vector contents doesn't look good; it is more reasonable either to push_back to the same vector,
or to iterate through non-skipped elements only.

Moved the one relevant only for RESERVRE as a lambda into `reserve(...)`.
Had to leave the second one (which does almost all the work of reserve) as a private helper
and document it.

Iterating (for example, via boost::filter_iterator + boost::transform_iterator) could be a
reasonable option with C++14; with C++11 this results in handwriting the return type of that
private helper, so I had to leave push_back in both places.


> On Dec. 4, 2019, 8:27 p.m., Benjamin Mahler wrote:
> > src/master/authorization.cpp
> > Lines 239-240 (patched)
> > <https://reviews.apache.org/r/71864/diff/1/?file=2181790#file2181790line239>
> >
> >     Ditto the broader question about validation errors from the previous review.

As far as I can tell, when authorizing RESERVE/UNRESERVE we have no real need for the logic
"let's authorize for any object if we did not compose any ActionObject".

There is an **exception**, though: when there is a mix of reservations with and without principals
in UNRESERVE, it can be argued that this mix is treated incorrectly (see https://jira.apache.org/jira/browse/MESOS-9562)
This patch implements a solution proposed in that ticket for UNRESERVE.


- Andrei


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


On Jan. 2, 2020, 8:01 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71864/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2020, 8:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10023 and MESOS-10056
>     https://issues.apache.org/jira/browse/MESOS-10023
>     https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> NOTE: This patch also partially addresses MESOS-9562 issue
> by modifying the logic of authorizing UNRESERVE operations.
> Now, if some reservations have no principal, both authorization of
> UNRESERVE_RESOURCES for ANY object and authorizations for unreserving
> all reservations that have principals will be requested.
> 
> 
> Diffs
> -----
> 
>   src/master/authorization.hpp PRE-CREATION 
>   src/master/authorization.cpp PRE-CREATION 
>   src/master/http.cpp 72587bf054e413402ebf4c6ff8060f7ca8ffcf1b 
>   src/master/master.hpp f97b085ae908278731acd326df68f9f381f09483 
>   src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 
> 
> 
> Diff: https://reviews.apache.org/r/71864/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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