> On March 14, 2017, 5:08 p.m., Anindya Sinha wrote: > > src/master/master.hpp > > Lines 686 (patched) > > <https://reviews.apache.org/r/57535/diff/2/?file=1664806#file1664806line686> > > > > nit: s/authorizeSlave/authorizeAgent?
This is for the sake of consistency. The master already/still has: ``` Master::addSlave Master::removeSlave ... ``` I think we are maintaining consistency within old files and will later do a sweeping change. This is of course sometimes less straightfoward: e.g., you have an internal method that directly corresponds to a new API which already uses the new terminology, etc. > On March 14, 2017, 5:08 p.m., Anindya Sinha wrote: > > src/master/master.cpp > > Line 5372 (original), 5446 (patched) > > <https://reviews.apache.org/r/57535/diff/2/?file=1664807#file1664807line5446> > > > > nit: Is there a specific reason for the change here (and in > > reregisterSlave) from `from` to `pid`? I am maintaining the pattern in the current `_registerSlave` and `_reregisterSlave`: https://github.com/apache/mesos/blob/5abaeec1ca53642bad329fce89fb3a03fbebc079/src/master/master.cpp#L5454 The way to look at it is: `registerSlave` is a message handler: the `from` here is injected by libprocess. In internal methods like `_registerSlave`, the meaning is changed: with ``` void _registerSlave( const SlaveInfo& slaveInfo, const process::UPID& pid, const Option<std::string>& principal, const std::vector<Resource>& checkpointedResources, const std::string& version, const std::vector<SlaveInfo::Capability>& agentCapabilities, const process::Future<bool>& authorized); ``` I am saying this method takes a `slaveInfo`, its associated pid is `pid`, its principal is `principal`, ... So `from` is not as needed / as clear anymore. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57535/#review168951 ----------------------------------------------------------- On March 14, 2017, 11:27 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57535/ > ----------------------------------------------------------- > > (Updated March 14, 2017, 11:27 a.m.) > > > Review request for mesos, Adam B, Anindya Sinha, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-7097 > https://issues.apache.org/jira/browse/MESOS-7097 > > > Repository: mesos > > > Description > ------- > > Applied RegisterAgent ACL to the master. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > e75e1879435f1c2bce47a86e9feebf9d051e969b > src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 > src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c > src/tests/master_authorization_tests.cpp > 1a0285a3f345ef21a8256d4123d8bb684f538da4 > > > Diff: https://reviews.apache.org/r/57535/diff/2/ > > > Testing > ------- > > make check. > > The tests added here cover some basic scenarios, I have more tests but will > add them when MESOS-7244 is fixed. > > > Thanks, > > Jiang Yan Xu > >
