mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 41444: Cleaned up Authorizer interface.
Date Fri, 18 Dec 2015 08:42:03 GMT

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


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)
<https://reviews.apache.org/r/41444/#comment171160>

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



include/mesos/authorizer/authorizer.hpp (lines 64 - 65)
<https://reviews.apache.org/r/41444/#comment171162>

    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)
<https://reviews.apache.org/r/41444/#comment171161>

    s/on/of/



include/mesos/authorizer/authorizer.hpp (line 69)
<https://reviews.apache.org/r/41444/#comment171163>

    s/autorizer/authorizer/
    s/on/of/



include/mesos/authorizer/authorizer.hpp (line 75)
<https://reviews.apache.org/r/41444/#comment171164>

    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)
<https://reviews.apache.org/r/41444/#comment171165>

    "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)
<https://reviews.apache.org/r/41444/#comment171166>

    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)
<https://reviews.apache.org/r/41444/#comment171168>

    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)
<https://reviews.apache.org/r/41444/#comment171167>

    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)
<https://reviews.apache.org/r/41444/#comment171169>

    s/RunTask/ShutdownFramework/



include/mesos/authorizer/authorizer.hpp (line 124)
<https://reviews.apache.org/r/41444/#comment171170>

    s/ran/registered/



include/mesos/authorizer/authorizer.hpp (line 133)
<https://reviews.apache.org/r/41444/#comment171171>

    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)
<https://reviews.apache.org/r/41444/#comment171172>

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



include/mesos/authorizer/authorizer.hpp (lines 140 - 141)
<https://reviews.apache.org/r/41444/#comment171173>

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


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