This is an automatically generated e-mail. To reply, visit:

Thank you for cleaning this up. It looked like an overwhelming amount of 
documentation for what is not really that complex of an API. It still looks a 
bit verbose/repetitive, so I've made some suggestions of what else to cut out.
I guess we're still waiting on the ACLs for create/remove persistent volumes, 
in MESOS-4179

include/mesos/authorizer/authorizer.hpp (line 40)

    s/logic/interpretation/? Or "meaning"?
    Or s/has the same logic/can be interpreted the same/

include/mesos/authorizer/authorizer.hpp (lines 64 - 65)

    How formal. I would think you could get away with
    s/the "authorizer.proto" file/"authorizor.proto"/
    s|the "docs/authorization.md"|"docs/authorization.md"|

include/mesos/authorizer/authorizer.hpp (line 66)


include/mesos/authorizer/authorizer.hpp (line 69)


include/mesos/authorizer/authorizer.hpp (line 75)

    What is "it"? Are we removing the initialize function, the acls parameter, 
or what?
    This seems very related to the first paragraph "Only relevant..." which 
should not be the first paragraph in the doxygen, since it is in no way a 
summary of the method.

include/mesos/authorizer/authorizer.hpp (lines 83 - 84)

    "The `principal` and `role` parameters are packed in the request." seems 
superfluous to the summary, and is already included in the 'request' @param. 
Delete this sentence.

include/mesos/authorizer/authorizer.hpp (line 100)

    Kill the "packed in" sentence for all of these. That info is in the request 
@param comment.

include/mesos/authorizer/authorizer.hpp (lines 102 - 104)

    How about just "`ACL::RunTask` packing all the parameters..."? We can 
figure out that it's an instance of a protobuf message.

include/mesos/authorizer/authorizer.hpp (lines 107 - 109)

    You can probably shorten this here and everywhere by just saying "A failed 
future indicates a problem processing the request; the request can be retried."

include/mesos/authorizer/authorizer.hpp (line 119)


include/mesos/authorizer/authorizer.hpp (line 124)


include/mesos/authorizer/authorizer.hpp (line 133)

    s/reserve particular resources/reserve resources/ since the only values 
currently allowed for `resources` are ANY or NONE.

include/mesos/authorizer/authorizer.hpp (line 138)

    s/reserve one or more types of resources/reserve resources/

include/mesos/authorizer/authorizer.hpp (lines 140 - 141)

    s/reserve the types of resources contained in the request/reserve resources/

- Adam B

On Dec. 16, 2015, 5:03 a.m., Alexander Rukletsov wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> -----------------------------------------------------------
> (Updated Dec. 16, 2015, 5:03 a.m.)
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> Repository: mesos
> Description
> -------
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> Diffs
> -----
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> Diff: https://reviews.apache.org/r/41444/diff/
> Testing
> -------
> None: not a functional change.
> Thanks,
> Alexander Rukletsov

Reply via email to