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 60821: Introduced a "no sender" UPID.
Date Fri, 14 Jul 2017 22:45:05 GMT


> On July 12, 2017, 5:27 p.m., James Peach wrote:
> > I think that a better approach is to define a static `UPID UPID::anonymous()` function
that returns a well-defined anonymous UPID for the system. Remove all the sending functions
that don't specify a `from` UPID and force them to explicitly pass the anonymous UPID. This
makes the used of anonymous message sends explicit which I think is preferable to this implicit
approach.
> 
> Jiang Yan Xu wrote:
>     Here's my thinking:
>     
>     - In libprocess the PID is usually not constructed directly, aside from being parsed
but that's also hidden from the users of the library. Users of libprocess tend to get the
PID from the process that owns that PID or from `spawn`. Afterwards it is used as an opaque
ID. Therefore I feel like more consistent with the current architecture to not expose it at
all. Having to know to use a speical ID is less straighfoward than calling a method that doesn't
take a `from` IMO.
>     
>     - Right now the PID is just a simple data struct and not guarded by a `process::initialize()`
call. This special UPID and PID uses the value of `__address__`, which initialized in `process::initialize()`.
Therefore even if we expose this anonymous / no sender PID directly, I feel it's probably
going to be constructed by something other than a static UPID method. This is a minor point
though.
> 
> James Peach wrote:
>     * Anything that calls `post` is going to have to initialize `libprocess`, so initializing
`libprocess` when we generate the anonymous UPID seems OK too. `UPID process::anonymous()`.
>     * Sending anonymous messages is a special case, so it should be explicit. `post`
is also a special case since you already have to specify a target UPID. I argue that explicit
is better than implicit and 1 way to `post` is better than 2.
> 
> Jiang Yan Xu wrote:
>     Yeah I think the main thing I am arguing for here is that as you said, the purpose
of this is to support "Sending anonymous messages / message with no sender address". IMO the
sentence from the user's POV translates to a method (and we already have this method), not
an UPID.
>     
>     In othe words, this special [process::post](https://github.com/apache/mesos/blob/5645fddf07eab0fcd195a3514e9e5ad3ef25628d/3rdparty/libprocess/include/process/process.hpp#L626-L637)
overload serves a special purpose that
>     - We currently have a use case directly
>     - I don't see how it could be incorrectly used for other unintended ways.
>     
>     
>     In contrast, exposing a special UPID to user:
>     - I don't see how the UPID can be used for another legit purpose.
>     - I can see it could used incorrectly by the user.
>     
>     
>     I do support explictness, but I think saying a method "sends message with no sender
address" is pretty explicit too and without exposing too much flexibility and a concept that
could be hard(er) to undertand (than this).
>     
>     Having said that, I am not adamant on the current design. Given another vote/idea
I can switch.
> 
> Joseph Wu wrote:
>     If the eventual goal is to enable `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH` by default,
it would make sense to either (A) add the special anonymous sender, or (B) remove the sender-less
`process::post` variant entirely.
>     
>     In terms of implementing (A), I would opt for an implementation that is explicit.
 This is primarily due to the ambiguity from calling `process::post(to, message)` within the
context of a libprocess Process, vs a main thread or test body.
>     
>     That being said, we don't have any protections or validation regarding the `from`
field anyway.  Any process can say `process::post(not_me /* :P */, to, message)` and that
would be considered valid.
>     
>     ---
>     
>     I agree with the explicit anonymous sender approach.  But I don't think we necessarily
need to implement the anonymous sender yet.
>     
>     I think going with the explicit route will give us an excuse to double-check any
places with no sender address/UPID.  If we see no reason to keep it, then we could add the
explicit anonymous sender if we ever find a reason to include it.

Not sure if I 100% understand the recommendation here. I think that it is:

- We implement `UPID process::anonymous()` and use it to avoid `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH`
check failure.

> But I don't think we necessarily need to implement the anonymous sender yet.

Did you mean to say that we do "(B) remove the sender-less process::post variant entirely"
later after we "double-check any places with no sender address/UPID"?


- Jiang Yan


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


On July 12, 2017, 5:04 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60821/
> -----------------------------------------------------------
> 
> (Updated July 12, 2017, 5:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7786
>     https://issues.apache.org/jira/browse/MESOS-7786
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The immediate use of this is to fix MESOS-7753 but I feel this is a general concept that's
defined in similar systems (e.g., akka).
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/process.cpp b4d7791782a5d9e10fa44489057c8504d594bab2 
>   3rdparty/libprocess/src/tests/process_tests.cpp c6109547d04bd06e9395e4ec5f5130fd1500f9a0

> 
> 
> Diff: https://reviews.apache.org/r/60821/diff/1/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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