----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60107/#review179132 -----------------------------------------------------------
Could you change the commit title to something like "Added filtering to the '/tasks' endpoint." ? src/common/http.hpp Lines 168 (patched) <https://reviews.apache.org/r/60107/#comment253635> Maybe something like: "Determines which objects will be accepted when filtering results based on authorization or other criteria." src/common/http.hpp Lines 180 (patched) <https://reviews.apache.org/r/60107/#comment253683> Let's call this `AuthorizationAcceptor` instead. Other names are OK I think. src/common/http.hpp Lines 184 (patched) <https://reviews.apache.org/r/60107/#comment253637> Insert a space before the `{}`, here and elsewhere. src/common/http.hpp Lines 205-218 (patched) <https://reviews.apache.org/r/60107/#comment253653> It doesn't look like this class is used at all in this patch, can we remove it? src/common/http.hpp Lines 228-229 (patched) <https://reviews.apache.org/r/60107/#comment253652> Should break after parentheses: ``` bool accept( const Task& task, ``` src/common/http.hpp Lines 242 (patched) <https://reviews.apache.org/r/60107/#comment253640> For conditionals, insert spaces like: ``` if (_frameworkId.isSome()) { ``` here and elsewhere. src/common/http.cpp Lines 1129-1132 (patched) <https://reviews.apache.org/r/60107/#comment253643> Let's do: ``` Future<Owned<AuthorizeFrameworkInfoAcceptor>> AuthorizeFrameworkInfoAcceptor::create( const Option<Principal>& principal, const Option<Authorizer*>& authorizer) ``` src/common/http.cpp Lines 1135-1138 (patched) <https://reviews.apache.org/r/60107/#comment253645> You should be able to return just an `Owned<AuthorizeFrameworkInfoAcceptor>` here, and the conversion to `Future` will happen implicitly: ``` return Owned<AuthorizeFrameworkInfoAcceptor>( new AuthorizeFrameworkInfoAcceptor( Owned<ObjectApprover>(new AcceptingObjectApprover()))); ``` here and elsewhere. src/common/http.cpp Lines 1141-1142 (patched) <https://reviews.apache.org/r/60107/#comment253646> You could probably just add a `using authorization::createSubject` statement to the beginning of this file to avoid always qualifying the namespace. Also, add a linebreak after this, here and elsewhere. src/common/http.cpp Lines 1143-1144 (patched) <https://reviews.apache.org/r/60107/#comment253647> Would prefer linebreaks like: ``` return authorizer.get()->getObjectApprover( subject, authorization::VIEW_FRAMEWORK) ``` here and elsewhere. src/common/http.cpp Lines 1145 (patched) <https://reviews.apache.org/r/60107/#comment253648> For lambdas, insert a space before the `{`, here and elsewhere. src/common/http.cpp Lines 1148 (patched) <https://reviews.apache.org/r/60107/#comment253649> The indentation of the lambda's closing `}` should match that of the line with the opening `{`, so indent this two more spaces. src/common/http.cpp Lines 1206 (patched) <https://reviews.apache.org/r/60107/#comment253684> Newline before this; here and elsewhere. src/master/http.cpp Line 3801 (original), 3801 (patched) <https://reviews.apache.org/r/60107/#comment253650> Can eliminate this comment. src/master/http.cpp Line 3806 (original), 3806 (patched) <https://reviews.apache.org/r/60107/#comment253659> How about `selectFrameworkId` instead of `acceptorFrameworkId`. src/master/http.cpp Line 3808 (original), 3808 (patched) <https://reviews.apache.org/r/60107/#comment253656> Indent two more spaces. src/master/http.cpp Line 3809 (original), 3809 (patched) <https://reviews.apache.org/r/60107/#comment253660> `selectTaskId` instead of `acceptorTaskId` src/master/http.cpp Line 3824 (original), 3823 (patched) <https://reviews.apache.org/r/60107/#comment253671> We can eliminate this comment. src/master/http.cpp Lines 3828-3832 (patched) <https://reviews.apache.org/r/60107/#comment253661> In this case, since `tie(` is 4 spaces long, you can do: ``` tie(authorizeFrameworkInfo, authorizeTask, acceptorFrameworkId, acceptorTaskId) = acceptors; ``` src/master/http.cpp Line 3831 (original), 3838 (patched) <https://reviews.apache.org/r/60107/#comment253663> The Framework struct has an `id()` method; let's use that here instead: `framework->id()` src/master/http.cpp Lines 3839 (patched) <https://reviews.apache.org/r/60107/#comment253662> Indent two more spaces, here and below. src/master/http.cpp Line 3848 (original), 3857 (patched) <https://reviews.apache.org/r/60107/#comment253672> This comment should mention unreachable tasks as well. src/tests/master_tests.cpp Line 3446 (original), 3448 (patched) <https://reviews.apache.org/r/60107/#comment253675> ``` Testing the '/master/tasks' endpoint without parameters, ``` src/tests/master_tests.cpp Lines 3451-3452 (patched) <https://reviews.apache.org/r/60107/#comment253676> Why is this necessary? The clock isn't paused in this test, so advancing explicitly shouldn't be necessary. src/tests/master_tests.cpp Lines 3449-3465 (original), 3459-3475 (patched) <https://reviews.apache.org/r/60107/#comment253682> I think the test will read a bit easier if you launch both tasks, then perform the two requests. - Greg Mann On June 27, 2017, 10:30 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60107/ > ----------------------------------------------------------- > > (Updated June 27, 2017, 10:30 p.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and > Vinod Kone. > > > Bugs: MESOS-7630 > https://issues.apache.org/jira/browse/MESOS-7630 > > > Repository: mesos > > > Description > ------- > > Finished object acceptor for "/tasks" endpoint. > > Added the query parameter support for that endpoint. > To query a task, you have to specify both framework_id > and task_id. Thus the query would look like > "/tasks?framework_id=[framework id];task_id=[task id]" > > > Diffs > ----- > > src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e > src/common/http.cpp 2f7718cbc2e449b4f7c89754e8f84eac2f3c35b6 > src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 > src/tests/master_tests.cpp 6cd4be7e5ba48c6562ce91b28e76b065522cfbea > > > Diff: https://reviews.apache.org/r/60107/diff/5/ > > > Testing > ------- > > Passed "make check" > Passed "GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48" > Passed "GLOG_v=1 ./bin/mesos-tests.sh > --gtest_filter="MasterTest.TasksEndpoint" --gtest_repeat=1000 > --gtest_break_on_failure" > > > Thanks, > > Quinn Leng > >
