Revert "Removed GrpcClientBuilder#MultiClientBuilder (#1809)" #2127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 74ac390.
There was a problem hiding this comment.
- Please describe in the commit message and PR description why we revert it.
- Consider adding a test that runs a single server with 2 different services, creates 2 clients with the same transport, sends one request to each service, verifies that only one connection is created. You can use TransportObserver to validate that.
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
Motivation: `GrpcClientBuilder#MultiClientBuilder` was previously removed, but it was the only practical way to share the underlying transport between different service API clients exposed by the same backend. At the same time, we don't want to expose ways to interact with the internal assembly of the transport layer, as we attach default filters and don't want to expose that internal implementation to users. Therefore, we're bringing the `MultiClientBuilder` back. Modifications: - Reverted `GrpcClientBuilder#MultiClientBuilder`, - Improved Javadoc, - Added `MultiClientTest` validating the behaviour. Result: `GrpcClientBuilder#MultiClientBuilder` can continue to be used by users migrating from 0.41 release and its' intention is clarified.
chemicL
force-pushed
the
restore-multi-client-builder
branch
from
84c17fb to
0406cb2
Compare
|
Thanks @idelpivnitskiy for the suggestion regarding a test. I added one. Will merge this PR. We can follow up if you see any further improvements are needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 74ac390.
Motivation:
GrpcClientBuilder#MultiClientBuilderwas previously removed, but itwas the only practical way to share the underlying transport between
different service API clients exposed by the same backend. At the same
time, we don't want to expose ways to interact with the internal assembly
of the transport layer, as we attach default filters and don't want to
expose that internal implementation to users. Therefore, we're bringing
the
MultiClientBuilderback.Modifications:
GrpcClientBuilder#MultiClientBuilder,MultiClientTestvalidating the behaviour.Result:
GrpcClientBuilder#MultiClientBuildercan continue to be used by usersmigrating from 0.41 release and its' intention is clarified.