> On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3604-3605 (original), 3604-3605 (patched) > > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3604> > > > > Isn't the empty case valid? Also, use `Resources::reservationRole`? > > Benjamin Bannier wrote: > With that helper we can probably even get rid of or reduce above comment > block and assertions, but should `CHECK(Resources::isReserved(resource))` > since `Resources::reservationRole` is only defined for reserved resources. > Still need to think about unreserved resources/empty case. > > Here and below.
The reason I explicitly set the role to `*` is for backward compatibility. Since now the resource is always in the post-reservation-refinement format, the empty case indicates that the resource is unreserved, and the old behavior is to look into `resource.role()` which would return the default value `*`. But, let me use the helpers to make it more readable: ``` const string role = Resources::isReserved(resources) ? Resources::reservationRole(resource) : "*"; ``` > On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Line 3662 (original), 3664 (patched) > > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3670> > > > > The `reservations.empty()` case is actually an invalid call right? > > There's nothing to unreserve. > > > > Should we be CHECKing that here or is that validated after this point > > today? Unfortunately, we validate the operation in `Master::_accept`, *after* getting authorizations: https://github.com/apache/mesos/blob/2c658770e7e273672d64fe6c7deaed05828e9a15/src/master/master.cpp#L4925, so we have to deal with this case. I'm just setting it to `*` for consistency and backward compatibility. Also see https://github.com/apache/mesos/blob/2c658770e7e273672d64fe6c7deaed05828e9a15/src/master/master.cpp#L3669. Let me add the comments related to validation back. > On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3723-3724 (original), 3723-3724 (patched) > > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3734> > > > > Is the empty case valid? > > > > Also, use `Resources::reservationRole`? See above. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69588/#review212027 ----------------------------------------------------------- On Dec. 19, 2018, 11:20 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, 11:20 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/2/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
