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