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