> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote: > > src/master/master.cpp > > Lines 4757-4758 (original), 4757-4758 (patched) > > <https://reviews.apache.org/r/69563/diff/1/?file=2113974#file2113974line4757> > > > > Performing this change in this patch makes it hard to review the > > important change. It should be in a separate patch. > > > > I personally am not sure we want to make this change as the reference > > semantics it introduces make it harder to verify that we e.g., do not > > access invalid references. This might not be worth the performance > > improvement this patch might bring. > > Chun-Hung Hsiao wrote: > That's why I have a `CHECK(authorizationIter != authorizations->end())` > everywhere before dereferencing. Since `authorizations` is const-qualified, > we won't be able to remove or modify any future, so in this aspect it's safer > compared to copying and poping futures. Note that in the original code we > never check if the deque is nonempty.
Moved the change to r/69571. > On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote: > > src/master/master.cpp > > Lines 5513-5519 (patched) > > <https://reviews.apache.org/r/69563/diff/1/?file=2113974#file2113974line5522> > > > > This patch now first authorizes and then validates. Is this the > > accepted pattern? In any case, this seems to be a change which would > > warrent its own patch. > > Chun-Hung Hsiao wrote: > This is the pattern we do for all other operations. And yes I'll create a > separated ticket for `LAUNCH_GROUP` and split this into another patch. Moved the change to r/69571. > On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote: > > src/master/master.cpp > > Lines 5805 (patched) > > <https://reviews.apache.org/r/69563/diff/1/?file=2113974#file2113974line5814> > > > > We should log some descriptive message here. Resolved in r/69571. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69563/#review211323 ----------------------------------------------------------- On Dec. 14, 2018, 1:55 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69563/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2018, 1:55 a.m.) > > > Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and > Jan Schlicht. > > > Bugs: MESOS-9474 > https://issues.apache.org/jira/browse/MESOS-9474 > > > Repository: mesos > > > Description > ------- > > This patch fixes a bug where the master does not complete the > processing of authorization results for `CREATE_DISK`, `DESTROY_DISK` > and `LAUNCH_GROUP`, causing other operations to drop if a remaining > authorization is denied.. > > > Diffs > ----- > > src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c > > > Diff: https://reviews.apache.org/r/69563/diff/1/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
