> On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > 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
... and set/remove quota, which are all committed now, except remove quota. > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, lines 64-65 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line64> > > > > 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"| Not sure what the second substitution means, but being inspired by the first sentence ("How formal"), I killed most of the comment : ). > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, line 75 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line75> > > > > 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. Good point! I moved the first paragraph here and refactored the comment. > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, lines 114-116 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line114> > > > > 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." Till suggested the following in [one of the reviews](https://reviews.apache.org/r/40346/): "A failed future however indicates a problem processing the request and the request can be retried." Almost identical to your proposal. > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, line 126 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line126> > > > > s/RunTask/ShutdownFramework/ Good catch! > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, line 144 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line144> > > > > s/reserve particular resources/reserve resources/ since the only values > > currently allowed for `resources` are ANY or NONE. I'd say, that's an implementation detail of `LocalAuthorizer`; AFAIK, we do not enforce it anywhere. I'm fine with leaving a `NOTE` though. What do you think? > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, line 153 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line153> > > > > s/reserve one or more types of resources/reserve resources/ See above. > On Dec. 18, 2015, 8:42 a.m., Adam B wrote: > > include/mesos/authorizer/authorizer.hpp, lines 155-156 > > <https://reviews.apache.org/r/41444/diff/1/?file=1166378#file1166378line155> > > > > s/reserve the types of resources contained in the request/reserve > > resources/ See above. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41444/#review111142 ----------------------------------------------------------- On Dec. 21, 2015, 3:42 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41444/ > ----------------------------------------------------------- > > (Updated Dec. 21, 2015, 3:42 p.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 > 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 > src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c > > Diff: https://reviews.apache.org/r/41444/diff/ > > > Testing > ------- > > None: not a functional change. > > > Thanks, > > Alexander Rukletsov > >
