You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Quinn Leng <qu...@gmail.com> on 2017/07/13 00:32:50 UTC

Re: Review Request 60822: Add filtering to /slaves, /slave/containers and /frameworks endpoints.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60822/
-----------------------------------------------------------

(Updated July 13, 2017, 12:32 a.m.)

Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod Kone.

Summary (updated)
-----------------

Add filtering to /slaves, /slave/containers and /frameworks endpoints.

Bugs: MESOS-7630
https://issues.apache.org/jira/browse/MESOS-7630

Repository: mesos

Description (updated)
-------

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

Add query parameter support for '/frameworks' endpoint.
To query the information about a specific framework, we
can use 'framework_id' to specify the framework, thus it
looks like'/frameworks?framework_id=[framework id]'

To maintain consistency, the structure for response is
reserved, thus the response still contains field 'frameworks'
and 'completed_frameworks' etc..

Since code for '/state' endpoint and '/frameworks' are coupled
(they share 'FullFrameworkWriter' class), thus the code for '/state'
endpoint is also updated.

Add query parameter support for '/slave/containers' endpoint.
To get the information about a specific container, you can specify
the container_id. The request would look like
'/slave/containers?container_id=[container id]'.

When no container id is specified, it will return the information
about all containers.

Diffs (updated)
-----

src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2
src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac
src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2

Diff: https://reviews.apache.org/r/60822/diff/2/

Changes: https://reviews.apache.org/r/60822/diff/1-2/

Testing
-------

Passed 'make check -j48'
Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check -j48'
Passed 'GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="MasterTest.FrameworksEndpointQueryFramework" --gtest_repeat=1000 --gtest_break_on_failure'
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

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

Posted by Greg Mann <gr...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60822/#review180750
-----------------------------------------------------------




src/master/http.cpp
Line 529 (original), 537-538 (patched)
<https://reviews.apache.org/r/60822/#comment255961>

    `&` should go next to the type:
    
    ```
      const Owned<AuthorizationAcceptor>& authorizeRole_;
      const SlaveIDAcceptor& selectSlaveId_;
    ```



src/master/http.cpp
Line 2467 (original), 2482 (patched)
<https://reviews.apache.org/r/60822/#comment255964>

    Indent this two more spaces.



src/master/http.cpp
Line 2793 (original), 2798 (patched)
<https://reviews.apache.org/r/60822/#comment255963>

    Indent this two more spaces.



src/slave/http.cpp
Lines 2109 (patched)
<https://reviews.apache.org/r/60822/#comment255970>

    Can we get rid of the `Owned` here?



src/slave/http.cpp
Lines 2131 (patched)
<https://reviews.apache.org/r/60822/#comment255975>

    Since `AuthorizationAcceptor::accept()` returns a `bool`, the `Try` is not necessary here. You could combine this check with the other two in the `if` statement above.


- Greg Mann


On July 14, 2017, 12:13 a.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> -----------------------------------------------------------
> 
> (Updated July 14, 2017, 12:13 a.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 query parameter support for the '/master/slaves',
> '/master/frameworks' and '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/4/
> 
> 
> Testing
> -------
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

Posted by Greg Mann <gr...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60822/#review180866
-----------------------------------------------------------




src/slave/http.cpp
Line 2131 (original), 2127 (patched)
<https://reviews.apache.org/r/60822/#comment256178>

    Indent this line one fewer space.


- Greg Mann


On July 18, 2017, 6:24 p.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> -----------------------------------------------------------
> 
> (Updated July 18, 2017, 6:24 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 query parameter support for the '/slaves', '/frameworks' and
> '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/6/
> 
> 
> Testing
> -------
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>