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 60107: Finished object acceptor for tasks endpoint.
Date Wed, 28 Jun 2017 19:05:25 GMT

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



Could you change the commit title to something like "Added filtering to the '/tasks' endpoint."
?


src/common/http.hpp
Lines 168 (patched)
<https://reviews.apache.org/r/60107/#comment253635>

    Maybe something like: "Determines which objects will be accepted when filtering results
based on authorization or other criteria."



src/common/http.hpp
Lines 180 (patched)
<https://reviews.apache.org/r/60107/#comment253683>

    Let's call this `AuthorizationAcceptor` instead. Other names are OK I think.



src/common/http.hpp
Lines 184 (patched)
<https://reviews.apache.org/r/60107/#comment253637>

    Insert a space before the `{}`, here and elsewhere.



src/common/http.hpp
Lines 205-218 (patched)
<https://reviews.apache.org/r/60107/#comment253653>

    It doesn't look like this class is used at all in this patch, can we remove it?



src/common/http.hpp
Lines 228-229 (patched)
<https://reviews.apache.org/r/60107/#comment253652>

    Should break after parentheses:
    
    ```
    bool accept(
        const Task& task,
    ```



src/common/http.hpp
Lines 242 (patched)
<https://reviews.apache.org/r/60107/#comment253640>

    For conditionals, insert spaces like:
    
    ```
    if (_frameworkId.isSome()) {
    ```
    
    here and elsewhere.



src/common/http.cpp
Lines 1129-1132 (patched)
<https://reviews.apache.org/r/60107/#comment253643>

    Let's do:
    
    ```
    Future<Owned<AuthorizeFrameworkInfoAcceptor>>
      AuthorizeFrameworkInfoAcceptor::create(
          const Option<Principal>& principal,
          const Option<Authorizer*>& authorizer)
    ```



src/common/http.cpp
Lines 1135-1138 (patched)
<https://reviews.apache.org/r/60107/#comment253645>

    You should be able to return just an `Owned<AuthorizeFrameworkInfoAcceptor>` here,
and the conversion to `Future` will happen implicitly:
    
    ```
    return Owned<AuthorizeFrameworkInfoAcceptor>(
        new AuthorizeFrameworkInfoAcceptor(
            Owned<ObjectApprover>(new AcceptingObjectApprover())));
    ```
    
    here and elsewhere.



src/common/http.cpp
Lines 1141-1142 (patched)
<https://reviews.apache.org/r/60107/#comment253646>

    You could probably just add a `using authorization::createSubject` statement to the beginning
of this file to avoid always qualifying the namespace.
    
    Also, add a linebreak after this, here and elsewhere.



src/common/http.cpp
Lines 1143-1144 (patched)
<https://reviews.apache.org/r/60107/#comment253647>

    Would prefer linebreaks like:
    
    ```
    return authorizer.get()->getObjectApprover(
        subject,
        authorization::VIEW_FRAMEWORK)
    ```
    
    here and elsewhere.



src/common/http.cpp
Lines 1145 (patched)
<https://reviews.apache.org/r/60107/#comment253648>

    For lambdas, insert a space before the `{`, here and elsewhere.



src/common/http.cpp
Lines 1148 (patched)
<https://reviews.apache.org/r/60107/#comment253649>

    The indentation of the lambda's closing `}` should match that of the line with the opening
`{`, so indent this two more spaces.



src/common/http.cpp
Lines 1206 (patched)
<https://reviews.apache.org/r/60107/#comment253684>

    Newline before this; here and elsewhere.



src/master/http.cpp
Line 3801 (original), 3801 (patched)
<https://reviews.apache.org/r/60107/#comment253650>

    Can eliminate this comment.



src/master/http.cpp
Line 3806 (original), 3806 (patched)
<https://reviews.apache.org/r/60107/#comment253659>

    How about `selectFrameworkId` instead of `acceptorFrameworkId`.



src/master/http.cpp
Line 3808 (original), 3808 (patched)
<https://reviews.apache.org/r/60107/#comment253656>

    Indent two more spaces.



src/master/http.cpp
Line 3809 (original), 3809 (patched)
<https://reviews.apache.org/r/60107/#comment253660>

    `selectTaskId` instead of `acceptorTaskId`



src/master/http.cpp
Line 3824 (original), 3823 (patched)
<https://reviews.apache.org/r/60107/#comment253671>

    We can eliminate this comment.



src/master/http.cpp
Lines 3828-3832 (patched)
<https://reviews.apache.org/r/60107/#comment253661>

    In this case, since `tie(` is 4 spaces long, you can do:
    
    ```
          tie(authorizeFrameworkInfo,
              authorizeTask,
              acceptorFrameworkId,
              acceptorTaskId) = acceptors;
    ```



src/master/http.cpp
Line 3831 (original), 3838 (patched)
<https://reviews.apache.org/r/60107/#comment253663>

    The Framework struct has an `id()` method; let's use that here instead: `framework->id()`



src/master/http.cpp
Lines 3839 (patched)
<https://reviews.apache.org/r/60107/#comment253662>

    Indent two more spaces, here and below.



src/master/http.cpp
Line 3848 (original), 3857 (patched)
<https://reviews.apache.org/r/60107/#comment253672>

    This comment should mention unreachable tasks as well.



src/tests/master_tests.cpp
Line 3446 (original), 3448 (patched)
<https://reviews.apache.org/r/60107/#comment253675>

    ```
    Testing the '/master/tasks' endpoint without parameters,
    ```



src/tests/master_tests.cpp
Lines 3451-3452 (patched)
<https://reviews.apache.org/r/60107/#comment253676>

    Why is this necessary? The clock isn't paused in this test, so advancing explicitly shouldn't
be necessary.



src/tests/master_tests.cpp
Lines 3449-3465 (original), 3459-3475 (patched)
<https://reviews.apache.org/r/60107/#comment253682>

    I think the test will read a bit easier if you launch both tasks, then perform the two
requests.


- Greg Mann


On June 27, 2017, 10:30 p.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60107/
> -----------------------------------------------------------
> 
> (Updated June 27, 2017, 10:30 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
> -------
> 
> Finished object acceptor for "/tasks" endpoint.
> 
> Added the query parameter support for that endpoint.
> To query a task, you have to specify both framework_id
> and task_id. Thus the query would look like
> "/tasks?framework_id=[framework id];task_id=[task id]"
> 
> 
> 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/5/
> 
> 
> 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
> 
>


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