You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/12/03 17:55:03 UTC
Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71864/
-----------------------------------------------------------
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
-------
Moved creating authorization Object out of `Master::authorize*reserve`.
Diffs
-----
src/master/authorization.hpp PRE-CREATION
src/master/authorization.cpp PRE-CREATION
src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778
src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
Diff: https://reviews.apache.org/r/71864/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> 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
>
>
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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.
>
> Andrei Sekretenko wrote:
> 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.
Should that ticket be linked into this review?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71864/#review218924
-----------------------------------------------------------
On Jan. 3, 2020, 2:12 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71864/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2020, 2:12 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71864/#review218924
-----------------------------------------------------------
src/master/authorization.cpp
Lines 111 (patched)
<https://reviews.apache.org/r/71864/#comment306852>
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?
src/master/authorization.cpp
Lines 239-240 (patched)
<https://reviews.apache.org/r/71864/#comment306850>
Ditto the broader question about validation errors from the previous review.
src/master/authorization.cpp
Lines 242 (patched)
<https://reviews.apache.org/r/71864/#comment306851>
be sure to look at the indents when you self-review before publishing :)
- Benjamin Mahler
On Dec. 3, 2019, 5:55 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71864/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 5:55 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
> -------
>
> Moved creating authorization Object out of `Master::authorize*reserve`.
>
>
> Diffs
> -----
>
> src/master/authorization.hpp PRE-CREATION
> src/master/authorization.cpp PRE-CREATION
> src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778
> src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
> src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
>
>
> Diff: https://reviews.apache.org/r/71864/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71864/#review219150
-----------------------------------------------------------
Ship it!
src/master/authorization.hpp
Lines 109-125 (patched)
<https://reviews.apache.org/r/71864/#comment307253>
I suppose this could have just been in the .cpp file rather than a static member exposed up in the header?
src/master/authorization.cpp
Lines 146-147 (patched)
<https://reviews.apache.org/r/71864/#comment307254>
seems a little more clear if it took the vector as an argument?
- Benjamin Mahler
On Jan. 3, 2020, 2:12 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71864/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2020, 2:12 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71864/
-----------------------------------------------------------
(Updated Jan. 3, 2020, 2:12 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Changes
-------
Fixed formatting in master.cpp
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/71864/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 71864: Moved creating authorization Object out of
`Master::authorize*reserve`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/71864/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko