> On May 7, 2016, 10:24 a.m., Adam B wrote: > > src/master/http.cpp, line 869 > > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line869> > > > > Did you need to pass all ("=") the in-scope variables through to the > > lambda, or just `request`? > > Jan Schlicht wrote: > Yes, I only need to capture `request` here, but the way I understand the > C++ style guide, "default capture by value" ("=") is preferred over "explicit > capture by value".
I'm not sure this is what the styleguide says. I read "Prefer default capture by value, explicit capture by value, then capture by reference." as explicit and default captures both take precedence over captures by reference. Anyway, if explicit capture is fine, this is the best use case for it: a lambda taking one parameter, which is exactly your case. Please change this. > On May 7, 2016, 10:24 a.m., Adam B wrote: > > src/master/http.cpp, line 2846 > > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line2846> > > > > There's a lot of code duplication between here and > > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the > > two? > > Jan Schlicht wrote: > Indeed, that's quite some code duplication. But there are also some > slight differences in both functions. Had a discussion with @arojas about how > to handle that case. On one hand it's good to remove code duplication, on the > other hand the extracted function would look a bit more complicated to > support the different requirements of master and agent. We decided that in > this particular case it's okay to stick with the two similar functions. If we split this function in two (per suggestion below), the copy-paste wouldn't be so drastic: we already have a lot of functions that do mapping principal->subject (it is unfortunate we have to repeat it again and again) and a fucntion that extracts endpoint from an URL path. Maybe we can add a helper in `Request` for this one. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46784/#review132125 ----------------------------------------------------------- On May 3, 2016, 12:42 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46784/ > ----------------------------------------------------------- > > (Updated May 3, 2016, 12:42 p.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Bannier. > > > Bugs: MESOS-5297 > https://issues.apache.org/jira/browse/MESOS-5297 > > > Repository: mesos > > > Description > ------- > > Access to the '/flags' endpoint is authorized using > the 'GET_ENDPOINT_WITH_PATH' action. > > > Diffs > ----- > > src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 > src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 > src/tests/master_authorization_tests.cpp > 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 > > Diff: https://reviews.apache.org/r/46784/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
