mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69588: Removed outdated authorization logic for offer operations.
Date Wed, 19 Dec 2018 20:15:31 GMT

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



Thanks for the cleanup! I found some of the comments potentially confusing for readers and
left some suggestions below.


src/master/master.cpp
Lines 3595-3597 (original), 3595-3597 (patched)
<https://reviews.apache.org/r/69588/#comment296559>

    We lost the comment here describing the invariant that we only support pushing one reservation
at a time, so all except the last have already been authorized and we only need to authorize
the last one on the stack.
    
    Can you preserve that comment?



src/master/master.cpp
Line 3597 (original), 3597 (patched)
<https://reviews.apache.org/r/69588/#comment296557>

    seems the use of `*` is more for backwards-compatiblity with the authorization protos
/ interface, rather than consistency?



src/master/master.cpp
Line 3600 (original), 3600 (patched)
<https://reviews.apache.org/r/69588/#comment296569>

    newline?



src/master/master.cpp
Lines 3607 (patched)
<https://reviews.apache.org/r/69588/#comment296558>

    this deprecated field.. being `value`?
    
    Maybe just say, we also set the deprecated value field for old authorizers that haven't
upgraded to look at resource?



src/master/master.cpp
Lines 3658-3659 (original), 3651-3652 (patched)
<https://reviews.apache.org/r/69588/#comment296560>

    This sounds confusing, it's not so clear to the reader here what the resources in `unreserve`
represent. (I'm left wondering if it's the `before` or `after` resources).
    
    From what I undertand, it's the `before` resources. Can we explain that?
    
    ```
    format. Since the unreserve operation only "pops" one level of reservation off the "stack",
we set the value to the principal of the outer-most reservation that will be unreserved (i.e.
"popped" off the stack).
    ```



src/master/master.cpp
Line 3661 (original), 3655 (patched)
<https://reviews.apache.org/r/69588/#comment296568>

    newline?



src/master/master.cpp
Lines 3662-3663 (patched)
<https://reviews.apache.org/r/69588/#comment296561>

    Ditto the suggestion above here.



src/master/master.cpp
Lines 3714-3716 (original), 3705-3707 (patched)
<https://reviews.apache.org/r/69588/#comment296562>

    This comment seems also a bit confusing for readers, and could use some "why" we do this,
now it just says "what" we do.



src/master/master.cpp
Line 3719 (original), 3710 (patched)
<https://reviews.apache.org/r/69588/#comment296567>

    newline?



src/master/master.cpp
Lines 3716-3717 (patched)
<https://reviews.apache.org/r/69588/#comment296563>

    Ditto here.



src/master/master.cpp
Lines 3759-3760 (patched)
<https://reviews.apache.org/r/69588/#comment296564>

    Ditto here.



src/master/master.cpp
Lines 3809-3814 (original), 3799-3804 (patched)
<https://reviews.apache.org/r/69588/#comment296565>

    Ditto here.



src/master/master.cpp
Line 3817 (original), 3807 (patched)
<https://reviews.apache.org/r/69588/#comment296566>

    newline?


- Benjamin Mahler


On Dec. 19, 2018, 7:38 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69588/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2018, 7:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Michael Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since the master now upgrades resources to the post-reservation-refinement
> format before authorization (see MESOS-7735), the authorization logic in the
> master becomes outdated. This patch cleans it up.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b4faf2b077a0288ba36195b7a21402932489d316 
> 
> 
> Diff: https://reviews.apache.org/r/69588/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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