mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 60581: Added filtering to the '/slaves' endpoint.
Date Wed, 12 Jul 2017 17:56:39 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message