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 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.
Date Fri, 02 Jun 2017 01:43:48 GMT

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




src/master/http.cpp
Lines 4148-4151 (patched)
<https://reviews.apache.org/r/59100/#comment250134>

    See Till's comment on previous RR regarding spacing here, to make it consistent with the
rest of this file.



src/master/http.cpp
Lines 4172-4173 (original), 4186-4194 (patched)
<https://reviews.apache.org/r/59100/#comment250136>

    Why not use `defer` here, instead of the double lambda? Something like:
    
    ```
    return approver.then(defer(
        master->self(),
        [=](Owned<ObjectApprover> approver) -> Future<Response> {
          const mesos::maintenance::Schedule schedule =
            _getMaintenanceSchedule(approver);
    
          return OK(JSON::protobuf(schedule), request.url.query.get("jsonp"));
        },
        lambda::_1));
    ```



src/master/http.cpp
Lines 4222-4240 (patched)
<https://reviews.apache.org/r/59100/#comment250143>

    Simply copying into a new `Schedule` message is inefficient, but probably OK for now,
as it's more readable and the output of this endpoint may not become too large.
    
    I'm mostly just leaving this comment for posterity, to note that we may want to use the
more sophisiticated JSON writer-based filtering in the future if performance becomes an issue,
as we do in the `/state` handler.



src/master/http.cpp
Lines 4331-4337 (original), 4383-4394 (patched)
<https://reviews.apache.org/r/59100/#comment250152>

    Ditto here regarding `defer`.



src/tests/api_tests.cpp
Lines 1263-1268 (patched)
<https://reviews.apache.org/r/59100/#comment250160>

    Ditto here, regarding changing from this callback-based pattern to the standard `AWAIT_EXPECT_RESPONSE_STATUS_EQ`
pattern.


- Greg Mann


On June 1, 2017, 2:57 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> -----------------------------------------------------------
> 
> (Updated June 1, 2017, 2:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
>     https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` and the action
> of the same name as the API call.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/4/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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