mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 57535: Applied RegisterAgent ACL to the master.
Date Wed, 15 Mar 2017 01:05:49 GMT


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


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