----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60581/#review180258 -----------------------------------------------------------
src/master/http.cpp Lines 2318 (patched) <https://reviews.apache.org/r/60581/#comment255338> Remove the comma at the end of this line. src/master/http.cpp Lines 2319 (patched) <https://reviews.apache.org/r/60581/#comment255337> s/specified/is specified/ src/master/http.cpp Lines 2320 (patched) <https://reviews.apache.org/r/60581/#comment255336> This empty string can be removed. src/master/http.cpp Lines 2329-2330 (original), 2334-2335 (patched) <https://reviews.apache.org/r/60581/#comment255426> For the `AuthorizationAcceptor`, we need to use an `Owned` because it holds onto an `Owned<ObjectApprover>`. However, in this case, I don't think there's a good reason to use `Owned<SlaveIDAcceptor>` rather than just `SlaveIDAcceptor`. You could do: ``` SlaveIDAcceptor selectSlaveID(request.url.query.get("slave_id")) ``` This is the case for other XXXXIDAcceptors as well - could you change other cases too? src/tests/master_tests.cpp Lines 2277-2278 (patched) <https://reviews.apache.org/r/60581/#comment255421> Maybe something like: "Ensures that the '/slaves' endpoint returns the correct slave when provided with a slave ID query parameter." src/tests/master_tests.cpp Lines 2282 (patched) <https://reviews.apache.org/r/60581/#comment255413> Newline after this. src/tests/master_tests.cpp Lines 2299-2300 (patched) <https://reviews.apache.org/r/60581/#comment255416> Fits on one line. src/tests/master_tests.cpp Lines 2345-2381 (patched) <https://reviews.apache.org/r/60581/#comment255418> I think we can test the filtering for just one of the slaves. src/tests/master_tests.cpp Lines 2383-2400 (patched) <https://reviews.apache.org/r/60581/#comment255422> Is this racy? You stop `slave1` but not `slave2`, and then you check that `slave2` is returned in 'recovered_slaves'. Isn't it possible that `slave2` reregisters? Should we terminate both slaves while the master is down? - Greg Mann On July 7, 2017, 10:36 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60581/ > ----------------------------------------------------------- > > (Updated July 7, 2017, 10:36 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 '/slaves' endpoint. > > Added query parameter support for 'slaves' > endpoint. We can use 'slave_id' to specify > which slave information to query. Thus it > looks like '/slaves?slave_id=[slave id]' > > If not slave id is specified, it will return > information of all slaves, just like before > > The structure for response is the same for both > specifying or not specifying slave id. Thus even > for request with slave id specified, the response > will still contain both fields 'slaves', > 'recovered_slaves' etc.. > > > Diffs > ----- > > src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a > src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 > src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a > src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f > > > Diff: https://reviews.apache.org/r/60581/diff/4/ > > > Testing > ------- > > Passed 'make check -j48' > Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48' > Passed 'GLOG_v=1 ./bin/mesos-tests.sh > --gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 > --gtest_break_on_failure' > > > Thanks, > > Quinn Leng > >
