> 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.
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. > 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. 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. > On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote: > > src/master/master.cpp > > Lines 5667 (patched) > > <https://reviews.apache.org/r/69563/diff/1/?file=2113974#file2113974line5676> > > > > This change and the one below seem in `DESTROY_DISK` seem to be the > > central pieces to address MESOS-9474. I'll split this into two patches. See above. - 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 > >
