----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60107/#review179195 -----------------------------------------------------------
src/common/http.hpp Lines 165 (patched) <https://reviews.apache.org/r/60107/#comment253783> s/criterias/criteria/ src/common/http.hpp Lines 166 (patched) <https://reviews.apache.org/r/60107/#comment253784> newline before "*/" src/common/http.hpp Lines 216-217 (patched) <https://reviews.apache.org/r/60107/#comment253788> Let's add a comment here that the default behavior of this acceptor when no ID is specified is to accept all inputs. For the TaskIDAcceptor as well. src/common/http.hpp Lines 220-227 (patched) <https://reviews.apache.org/r/60107/#comment253787> Could you move the constructor definition into the implementation file? Also for the `TaskIDAcceptor`. src/common/http.cpp Lines 1136-1137 (patched) <https://reviews.apache.org/r/60107/#comment253791> Indented too far. src/common/http.cpp Lines 1141 (patched) <https://reviews.apache.org/r/60107/#comment253790> Indented too far. src/common/http.cpp Lines 1154 (patched) <https://reviews.apache.org/r/60107/#comment253786> Newline in between the function arguments src/common/http.cpp Lines 1158-1159 (patched) <https://reviews.apache.org/r/60107/#comment253789> Indented too far. src/common/http.cpp Lines 1184 (patched) <https://reviews.apache.org/r/60107/#comment253792> You can eliminate these comments. src/common/http.cpp Lines 1192 (patched) <https://reviews.apache.org/r/60107/#comment253785> Newline after `(` src/master/http.cpp Lines 3817-3877 (original), 3811-3887 (patched) <https://reviews.apache.org/r/60107/#comment253778> Could you update this section to match the indentation we were discussing? src/master/http.cpp Lines 3822-3823 (original), 3820-3821 (patched) <https://reviews.apache.org/r/60107/#comment253794> These would fit together on one line. src/master/http.cpp Line 3823 (original), 3821 (patched) <https://reviews.apache.org/r/60107/#comment253776> Space before `{` src/master/http.cpp Lines 3826-3830 (patched) <https://reviews.apache.org/r/60107/#comment253777> Since the identation is aligned, you don't need the newline after `(`: ``` tie(authorizeFrameworkInfo, authorizeTask, selectFrameworkId, selectTaskId) = acceptors; ``` src/tests/master_tests.cpp Lines 3440 (patched) <https://reviews.apache.org/r/60107/#comment253795> Indented too far. src/tests/master_tests.cpp Lines 3495 (patched) <https://reviews.apache.org/r/60107/#comment253765> "the '/master/tasks' endpoint" src/tests/master_tests.cpp Lines 3525-3526 (patched) <https://reviews.apache.org/r/60107/#comment253767> Newline here src/tests/master_tests.cpp Lines 3545-3547 (patched) <https://reviews.apache.org/r/60107/#comment253766> We want to assert that both of these tasks are there. Should do ``` ASSERT_TRUE(value->contains(expected1.get())); ASSERT_TRUE(value->contains(expected2.get())); ``` src/tests/master_tests.cpp Lines 3550 (patched) <https://reviews.apache.org/r/60107/#comment253768> Period at the end of the comment. src/tests/master_tests.cpp Lines 3554 (patched) <https://reviews.apache.org/r/60107/#comment253775> Can use the `->` operator: ``` frameworkId->value() ``` src/tests/master_tests.cpp Lines 3564 (patched) <https://reviews.apache.org/r/60107/#comment253769> Let's use the name `object` instead of abbreviating `obj`. src/tests/master_tests.cpp Lines 3568 (patched) <https://reviews.apache.org/r/60107/#comment253772> Can eliminate this comment, since the code is clear enough. src/tests/master_tests.cpp Lines 3569-3571 (patched) <https://reviews.apache.org/r/60107/#comment253774> You already assert that `taskArray.isSome()` above, so no need for this conditional. Also, you can use the `->` operator: ``` EXPECT_TRUE(taskArray->values.size() == 1); ``` - Greg Mann On June 28, 2017, 10:05 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60107/ > ----------------------------------------------------------- > > (Updated June 28, 2017, 10:05 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 > ------- > > Added filtering to the '/tasks' endpoint. > > > 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/7/ > > > 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 > >
