Tcp: add a tcp async client #25079
Tcp: add a tcp async client #25079
Conversation
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
|
/assign @yanavlasov Thanks for the review! For the test filter, I am not using the filter owned buffer, but directly use the underlying connection buffer to achieve the stream. |
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@yanavlasov PTAL, thanks! |
|
I have pinged maintainers for non Google review |
| @@ -523,6 +525,8 @@ class ClusterManagerImpl : public ClusterManager, | |||
| LoadBalancerContext* context) override; | |||
| Host::CreateConnectionData tcpConn(LoadBalancerContext* context) override; | |||
| Http::AsyncClient& httpAsyncClient() override; | |||
| Tcp::AsyncTcpClientPtr tcpAsyncClient(LoadBalancerContext* context, | |||
There was a problem hiding this comment.
The AsyncTcpClient has different semantics from the HTTP async client so it might be a bit confusing. While the HTTP async client allows you to start multiple stream/request to the cluster, the AsyncTcpClient is bound to 1 connection and not reusable.
There was a problem hiding this comment.
Right, this is a factory method of the AsyncTcpClient, do you have some other suggestions?.
There was a problem hiding this comment.
That depends on how this is going to be used, you might want follow the same pattern of the HTTP async client that starting multiple streams (connections in TCP).
There was a problem hiding this comment.
I see, 1 connection works for our use case right now, and I am thinking the client factory can be configurable e.g., we can add multiple connections support either in the current object or a new implementation of the interface in future. Does that SG? Thanks!
There was a problem hiding this comment.
SGTM, what I'm saying is that the name convention here confuses a bit compared to the http client above. The benefit of the http client is that you don't need to store/pass around a ClusterManager. In your use case, what if you want to retry that 1 connection?
There was a problem hiding this comment.
Thanks! We don't retry right now, I added an option to the factory, and then user can decide how the client is created & used.
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao boteng@google.com
Add a Tcp async client and factory to thread local cluster.
Commit Message:
Additional Description:
Risk Level: Low
Testing: integration & unit test
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]