----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39989/#review107281 -----------------------------------------------------------
src/master/master.cpp (lines 3016 - 3028) <https://reviews.apache.org/r/39989/#comment166313> Let's move the validation to aurhtorizeReserveResources. You also need to add a NOTE in `_accept` saying that no need to validate the operation again since it's already validated during authorization. src/master/master.cpp (lines 3053 - 3054) <https://reviews.apache.org/r/39989/#comment166312> I don't think the comments here is necessary. We need to avoid over-commenting. If the code itself is quite clear about what it's about to do, no need for the extra comments here. src/master/master.cpp (line 3160) <https://reviews.apache.org/r/39989/#comment166314> No need for this comment. src/master/master.cpp (line 3195) <https://reviews.apache.org/r/39989/#comment166311> I don't think this comment add more value. The code itself is quite clear what it is about to do. src/master/master.cpp (line 3202) <https://reviews.apache.org/r/39989/#comment166315> Ditto. src/master/master.cpp (line 3206) <https://reviews.apache.org/r/39989/#comment166316> Remove this comment. src/master/master.cpp (line 3237) <https://reviews.apache.org/r/39989/#comment166317> Ditto. src/master/master.cpp (line 3310) <https://reviews.apache.org/r/39989/#comment166318> Ditto. src/tests/reservation_tests.cpp (lines 1657 - 1668) <https://reviews.apache.org/r/39989/#comment166320> This might be flaky because master might send out an offer before it processes the terminal status update. - Jie Yu On Nov. 20, 2015, 12:20 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39989/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2015, 12:20 a.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff. > > > Bugs: MESOS-3062 > https://issues.apache.org/jira/browse/MESOS-3062 > > > Repository: mesos > > > Description > ------- > > Added framework authorization for dynamic reservation. > Note: this review is continued from https://reviews.apache.org/r/37127/ > > > Diffs > ----- > > src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 > src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 > > Diff: https://reviews.apache.org/r/39989/diff/ > > > Testing > ------- > > This is the fifth in a chain of 5 patches. New reservation tests were added > to `reservation_tests.cpp` to validate the authentication of framework > reserve and unreserve operations using ACLs. `make check` was run to test > after all patches were applied. > > > Thanks, > > Greg Mann > >
