You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2017/08/02 12:38:33 UTC
Re: Review Request 61271: Implemented HTTP connection handling for the
resource provider driver.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61271/
-----------------------------------------------------------
(Updated Aug. 2, 2017, 2:38 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed most issues (retry logic still missing).
Bugs: MESOS-7469
https://issues.apache.org/jira/browse/MESOS-7469
Repository: mesos
Description
-------
Similar to the existing HTTP connection handling of schedulers and
executors, the resource provider driver will create two connections
with the resource provider manager, one for streaming events and another
one for sending calls. This connection handling has been generalized as
a 'HttpConnectionProcess' and can be reused in other cases.
Diffs (updated)
-----
include/mesos/v1/resource_provider.hpp 88b606212ea57fee1c1ea522d2dc7f8124a9adef
src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9
src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05
src/resource_provider/http_connection.hpp PRE-CREATION
src/resource_provider/storage/provider.cpp 4c39312be5e4a6d783df3d385a66be6b3dcf8603
Diff: https://reviews.apache.org/r/61271/diff/3/
Changes: https://reviews.apache.org/r/61271/diff/2-3/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 61271: Implemented HTTP connection handling for the
resource provider driver.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > include/mesos/v1/resource_provider.hpp
> > Lines 81 (patched)
> > <https://reviews.apache.org/r/61271/diff/3/?file=1788172#file1788172line82>
> >
> > Passing an `Option` here makes sense to me, but this is not done in e.g., the scheduler, https://github.com/apache/mesos/blob/382f526ee2c13df063e17d8346915f3716fe6d21/include/mesos/scheduler.hpp#L364-L366 and I am not sure I follow their reasoning (we also already assume that e.g., libprocess can be used in this interface).
`mesos/scheduler.hpp` isn't the right place to look, it's actually `mesos/v1/scheduler.hpp`. There, an `Option` is used as a parameter in the constructor. Maybe stout isn't visible in `mesos/scheduler.hpp` because it's a public interface that is used by other language bindings.
> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 1095 (patched)
> > <https://reviews.apache.org/r/61271/diff/3/?file=1788173#file1788173line1095>
> >
> > We need this in the cmake setup as well.
I couldn't find a similar section in any of the `CMakeLists.txt`, that's why I didn't add anything there.
> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/resource_provider/driver.cpp
> > Lines 43 (patched)
> > <https://reviews.apache.org/r/61271/diff/3/?file=1788174#file1788174line45>
> >
> > As we only have a single user below, I feel that it would be fine to use a lambda at the use site instead.
The use site is a constructor's initializer list. While I could use a lambda there, IMHO it doesn't look very nice and quite distracting.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61271/#review182082
-----------------------------------------------------------
On Aug. 2, 2017, 2:38 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61271/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2017, 2:38 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to the existing HTTP connection handling of schedulers and
> executors, the resource provider driver will create two connections
> with the resource provider manager, one for streaming events and another
> one for sending calls. This connection handling has been generalized as
> a 'HttpConnectionProcess' and can be reused in other cases.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp 88b606212ea57fee1c1ea522d2dc7f8124a9adef
> src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9
> src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05
> src/resource_provider/http_connection.hpp PRE-CREATION
> src/resource_provider/storage/provider.cpp 4c39312be5e4a6d783df3d385a66be6b3dcf8603
>
>
> Diff: https://reviews.apache.org/r/61271/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 61271: Implemented HTTP connection handling for the
resource provider driver.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 3, 2017, noon, Benjamin Bannier wrote:
> > src/resource_provider/http_connection.hpp
> > Lines 90 (patched)
> > <https://reviews.apache.org/r/61271/diff/3/?file=1788175#file1788175line90>
> >
> > I wonder if it makes more sense to return e.g., a `Try<bool>` here in order to surface the failure scenarios below, many of which might be caused by the caller. This could give them a chance to retry if we map `true` to success and `false` to transient errors.
> >
> > Otherwise let's add a `TODO`.
It's returning a `Future<Nothing>` now.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61271/#review182082
-----------------------------------------------------------
On Aug. 18, 2017, 12:18 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61271/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 12:18 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to the existing HTTP connection handling of schedulers and
> executors, the resource provider driver will create two connections
> with the resource provider manager, one for streaming events and another
> one for sending calls. This connection handling has been generalized as
> a 'HttpConnectionProcess' and can be reused in other cases.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp 88b606212ea57fee1c1ea522d2dc7f8124a9adef
> src/CMakeLists.txt 98ccaf41bf5e7d14164e1c8b6e49268ac865a52c
> src/Makefile.am 68fff148f3d0c1710305bd9afcba62336d194b55
> src/resource_provider/detector.hpp PRE-CREATION
> src/resource_provider/detector.cpp PRE-CREATION
> src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05
> src/resource_provider/http_connection.hpp PRE-CREATION
> src/resource_provider/storage/provider.cpp 4c39312be5e4a6d783df3d385a66be6b3dcf8603
>
>
> Diff: https://reviews.apache.org/r/61271/diff/6/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 61271: Implemented HTTP connection handling for the
resource provider driver.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61271/#review182082
-----------------------------------------------------------
include/mesos/v1/resource_provider.hpp
Lines 37-39 (patched)
<https://reviews.apache.org/r/61271/#comment257930>
Let's move this into the namespace `mesos` declared below.
nit: `} // namespace internal {
include/mesos/v1/resource_provider.hpp
Lines 81 (patched)
<https://reviews.apache.org/r/61271/#comment257931>
Passing an `Option` here makes sense to me, but this is not done in e.g., the scheduler, https://github.com/apache/mesos/blob/382f526ee2c13df063e17d8346915f3716fe6d21/include/mesos/scheduler.hpp#L364-L366 and I am not sure I follow their reasoning (we also already assume that e.g., libprocess can be used in this interface).
src/Makefile.am
Lines 1095 (patched)
<https://reviews.apache.org/r/61271/#comment257932>
We need this in the cmake setup as well.
src/resource_provider/driver.cpp
Lines 43 (patched)
<https://reviews.apache.org/r/61271/#comment257937>
As we only have a single user below, I feel that it would be fine to use a lambda at the use site instead.
src/resource_provider/driver.cpp
Lines 52-63 (original), 50-59 (patched)
<https://reviews.apache.org/r/61271/#comment257935>
Let's not introduce this as part of this patch, but instead as a separate package implementing https://issues.apache.org/jira/browse/MESOS-7854 (we should also add tests for this functionality then). Currently this assume a certain authentication scheme which might not what we want.
We can pass a none credential when instantiating the driver below.
src/resource_provider/driver.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/61271/#comment257936>
Just pass `None()`, see above.
src/resource_provider/http_connection.hpp
Lines 59 (patched)
<https://reviews.apache.org/r/61271/#comment257945>
Let's document somewhere our expectations on `Call`. We e.g., assume that it has an enum-valued member function `type()`. The enum value `Call::SUBSCRIBE` has a special meaning. Pretty minor, we also expect the enum value to be stringifyable.
src/resource_provider/http_connection.hpp
Lines 82 (patched)
<https://reviews.apache.org/r/61271/#comment257942>
Let's add this as part of MESOS-7854 (we do not have any tests for it ATM anyway).
src/resource_provider/http_connection.hpp
Lines 90 (patched)
<https://reviews.apache.org/r/61271/#comment257943>
I wonder if it makes more sense to return e.g., a `Try<bool>` here in order to surface the failure scenarios below, many of which might be caused by the caller. This could give them a chance to retry if we map `true` to success and `false` to transient errors.
Otherwise let's add a `TODO`.
src/resource_provider/http_connection.hpp
Lines 99-113 (patched)
<https://reviews.apache.org/r/61271/#comment257946>
This block ensures both that callers use this method correctly, and that our internal invariants are good.
Explicitly calling out the internal invariants could make this more readable, e.g., add below
CHECK(state == State::CONNECTED || state == State::SUBSCRIBED) << state;
CHECK_SOME(connections);
src/resource_provider/http_connection.hpp
Lines 129 (patched)
<https://reviews.apache.org/r/61271/#comment257947>
Suggest to move this just below the block ensuring this does not fire.
src/resource_provider/http_connection.hpp
Lines 133 (patched)
<https://reviews.apache.org/r/61271/#comment257948>
Let's call out what state transition we are performing, e.g.,
CHECK_EQ(State::CONNECTED, state);
src/resource_provider/http_connection.hpp
Lines 237-241 (patched)
<https://reviews.apache.org/r/61271/#comment257944>
We check an aweful lot of states here making it hard to see what cases could be missed. How about
void disconnected(const std::string& failure)
{
switch (state) {
case State::DISCONNECTED: {
UNREACHABLE();
}
case State::CONNECTED:
case State::CONNECTING:
case State::SUBSCRIBED: {
mutex.lock()
.then(defer(
self(),
[this]() { return process::async(callbacks.disconnected); }))
.onAny(lambda::bind(&process::Mutex::unlock, mutex));
break;
}
case State::SUBSCRIBING: {
break;
}
}
disconnect();
}
src/resource_provider/http_connection.hpp
Lines 400 (patched)
<https://reviews.apache.org/r/61271/#comment257939>
Let's delete the copy constructor here, e.g.,
// The decoder cannot be copied meaningfully, see MESOS-5122.
SubscribedResponse(const SubscribedResponse&) = delete;
SubscribedResponse(SubscribedResponse&&) = default;
src/resource_provider/http_connection.hpp
Lines 408-409 (patched)
<https://reviews.apache.org/r/61271/#comment257938>
Nothing agent-specific here, what about e.g., `s/agent/remote side/` or similar?
src/resource_provider/http_connection.hpp
Lines 431 (patched)
<https://reviews.apache.org/r/61271/#comment257941>
Let's do this as part of MESOS-7854 (we do not have any tests for it ATM anyway).
src/resource_provider/storage/provider.cpp
Lines 127 (patched)
<https://reviews.apache.org/r/61271/#comment257934>
As superfically the interfaces look like we already do authn/z let's add a `TODO`, e.g., here. We can reference https://issues.apache.org/jira/browse/MESOS-7854.
- Benjamin Bannier
On Aug. 2, 2017, 2:38 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61271/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2017, 2:38 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7469
> https://issues.apache.org/jira/browse/MESOS-7469
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to the existing HTTP connection handling of schedulers and
> executors, the resource provider driver will create two connections
> with the resource provider manager, one for streaming events and another
> one for sending calls. This connection handling has been generalized as
> a 'HttpConnectionProcess' and can be reused in other cases.
>
>
> Diffs
> -----
>
> include/mesos/v1/resource_provider.hpp 88b606212ea57fee1c1ea522d2dc7f8124a9adef
> src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9
> src/resource_provider/driver.cpp 6778ec9c863022446f141ee88f70eb563178ea05
> src/resource_provider/http_connection.hpp PRE-CREATION
> src/resource_provider/storage/provider.cpp 4c39312be5e4a6d783df3d385a66be6b3dcf8603
>
>
> Diff: https://reviews.apache.org/r/61271/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>