----------------------------------------------------------- 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 > >
