mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 61271: Implemented HTTP connection handling for the resource provider driver.
Date Thu, 03 Aug 2017 10:00:27 GMT

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


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