Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wip: Add multi-connection support in H2 conn pool. #7852

Closed
wants to merge 5 commits into from
Closed

Wip: Add multi-connection support in H2 conn pool. #7852

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb7a9b8
Wip: Add multi-connection support in H2 conn pool.
conqerAtapple Jul 8, 2019
e266525
Added comments and TODOs.
conqerAtapple Aug 7, 2019
ce362fb
Removed references to old clients.
conqerAtapple Aug 8, 2019
ee3fe0c
Cleanup.
conqerAtapple Aug 9, 2019
63004e2
Changes:
conqerAtapple Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions include/envoy/upstream/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,51 @@ using ProtocolOptionsConfigConstSharedPtr = std::shared_ptr<const ProtocolOption
*/
class ClusterTypedMetadataFactory : public Envoy::Config::TypedMetadataFactory {};

/**
* Interface for connection policy subscriber. `ConnectionRequestPolicy` uses
* this interface as a contract between the policy implementation and policy
* user(subscriber).
*/
class ConnectionRequestPolicySubscriber {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add interface comments for these classes and functions? It's not immediately clear to me why these policies need to live in upstream.h. Can we potentially move them to a new file? I'm not sure how they are going to get configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added comments and TODOs hoping to make the intent clearer. There is no strong reason for having the policy interfaces in upstream.h. The only reason i went with it for now is due to its proximity to ClusterInfo. Since the policies are related (1:1) to ClusterInfo, they could be declared together. I am open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks this is more clear. IMO I would simplify this for right now and not worry about a public interface, cluster info, etc. I would probably just build this config directly into the a new http2 connection pool options config:

envoy/api/envoy/api/v2/cds.proto

Line 252 in e03936e

core.Http2ProtocolOptions http2_protocol_options = 14;
. You can then plumb that config directly from cluster info -> the http2 connection pool much like we do for the existing http2 protocol options.

In terms of these interfaces and implementation, I would either just define them right now directly in the http2 conn pool code, or make new files in that area to contain them. We can always make this more generic later but IMO it's fine to define this where we are going to use this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 Thanks! How do you visualize the Htt2ConnectionPolicy config? I suppose something like

 clusters:                                                                      
    - name: cluster-name                                                   
      connect_timeout: 0.25s       
      connection_policy:
           max_requests_per_connection: 1  
           drain_on_overflow: yes // Should drain or keep in overflow list
           idle_timeout: 500ms     // Time after which the connection can be closed.                   
                                                  
      http2_protocol_options: {...}
      ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this yes, but per @alyssawilk we should see if we can also unify with HTTP/1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a super cursory pass at this and while I'm really happy to get this work into HTTP/2 I'm worried about the code overlap between the HTTP/1 multiple-connection management and HTTP/2.

I think we might do better here if we tried to factor out / enhance the connection management we already have, and then use it in the H2 connection pool. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this. This was going to be my suggestion as well once we get the code a bit more stabilized. I think there will be significant duplication. Up to you whether you want to try to do it in HTTP/2 first and then figure out the overlap or try to factor out first... (There already is a common base class you can work from.)

public:
virtual ~ConnectionRequestPolicySubscriber() = default;
// Number of requests currently attached to the subscriber.
virtual uint64_t requestCount() const PURE;

// Resource manager used by the subscriber.
virtual ResourceManager& resourceManager() const PURE;
};

/**
* Interface for connection policy. This policy controls the states to
* transition a connection policy subscriber on:
* - a new stream (request) attached to a connection policy subscriber(a connection
* implemention for all practical purposes).
* - on reset of a stream(request) in the connection.
*
* Policies could be :
* - Aggregate request count based: Total requests attached till now.
* - Current requests count based: Number of requests currently attached.
*/
class ConnectionRequestPolicy {
public:
enum class State {
Init, // Initial
Ready, // Ready to accept new streams(requests)
Active, // At least one stream(request) is attached
Overflow, // Exceeded max allowed streams(requests)
Drain // Marked for close. Do not attach any more stream (request)
};

virtual ~ConnectionRequestPolicy() = default;

// State transition on attaching a new stream(request) to a policy subscriber.
virtual State onNewStream(const ConnectionRequestPolicySubscriber&) const PURE;

// State transition on detaching stream(request) from a policy subscriber.
virtual State onStreamReset(const ConnectionRequestPolicySubscriber&, const State&) const PURE;
};

/**
* Information about a given upstream cluster.
*/
Expand Down Expand Up @@ -848,6 +893,11 @@ class ClusterInfo {
*/
virtual void createNetworkFilterChain(Network::Connection& connection) const PURE;

/*
* Connection policy for the cluster.
*/
virtual const ConnectionRequestPolicy& connectionPolicy() const PURE;

protected:
/**
* Invoked by extensionProtocolOptionsTyped.
Expand Down
Loading