Channel.authority() should return the value of overrideAuthority #2956
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
|
FYI: most of the time we'll rebase instead of merging, and then squash before pushing to the master branch. |
ejona86
reviewed
Apr 27, 2017
| * functionality. | ||
| */ | ||
| @VisibleForTesting | ||
| protected static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { |
There was a problem hiding this comment.
package-private instead of protected?
| @@ -308,6 +308,10 @@ public ManagedChannel build() { | |||
| // getResource(), then this shouldn't be a problem unless called on the UI thread. | |||
| nameResolverFactory = NameResolverProvider.asFactory(); | |||
| } | |||
There was a problem hiding this comment.
Delete AuthorityOverridingTransportFactory?
There was a problem hiding this comment.
Didn't realize that this was all that AuthorityOverridingTransportFactory was trying to accomplish. Done.
|
|
||
| /** | ||
| * Constructor for the {@link NameResolver.Factory} | ||
| * @param delegate The actual underlying factory that will produce the a {@link NameResolver} |
There was a problem hiding this comment.
Leave an empty line before the section with @param and similar.
| */ | ||
| @VisibleForTesting | ||
| protected static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { | ||
| final NameResolver.Factory delegate; |
There was a problem hiding this comment.
Private? Ditto for authorityOverride.
|
|
||
| import java.net.InetSocketAddress; | ||
| import java.net.URI; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
There was a problem hiding this comment.
Creating extra sections here is the old style. Now there are only two sections: static imports and normal imports.
…should be private
zpencer
force-pushed
the
zpencer/2682/overrideAuthority
branch
from
1660ff7 to
d6f0926
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
AbstractManagedChannelImplBuilder accepts an overrideAuthority parameter, but this value is not hooked up to the name resolver object. Ultimately, Channel.authority consults with the NameResolver, so the overrideAuthority should be hooked into the NameResolverFactory, while all other functionality should be preserved.
Also, add unit tests for all the variants of OkHttpChannelBuilder and NettyChannelBuilder constructors, namely to test the slightly different NettyChannelBuilder(SocketAddress) code path.
Fixes #2682