Introduce NameResolver.Args.Extensions #11669
Conversation
jdcormie
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
|
Friendly 1 week ping. This is the domain-specific Key<>-based approach you requested in our last meeting. |
| * | ||
| * <p>Optional, {@link Extensions#EMPTY} by default. | ||
| */ | ||
| public Builder setExtensions(Extensions extensions) { |
There was a problem hiding this comment.
Hmmm... This makes Extensions public, and you have to set all Extensions for this builder but reading gets only a single one. I see the awkwardness is because we're going through the ManagedChannelBuilder API. I'm tempted to just use a map in the channel builder and copy it to this API later.
There was a problem hiding this comment.
Completely removed Extensions from the public API (it's now @internal)
I tried maintaining a map in ManagedChannelImplBuilder but the code to copy entries over was problematic because 1) one more place needing unchecked casts 2) it's doing work that's best encapsulated and unit tested inside Args.Extensions.
I also tried out moving the NameResolver.Args.Builder to ManagedChannelImplBuilder. This worked but only because the current ManagedChannelImpl ctor unconditionally sets every property of Args.Builder. This is fragile - if this code ever changed to conditionally set a property it would silently leak that property across channels when the builder is reused. This seemed too fragile to me.
|
|
||
| private IdentityHashMap<Key<?>, Object> data(int size) { | ||
| if (newdata == null) { | ||
| newdata = new IdentityHashMap<>(size); |
There was a problem hiding this comment.
Would it be nicer to copy base at this point:
newdata = new IdentityHashMap<>(base.data);Yes, you're doing the sizing of the map here, but that size is pretty arbitrary. If we care:
newdata = new IdentityHashMap<>(base.data.size() + 1);
newdata.putAll(base.data);There was a problem hiding this comment.
Hmm seems like that could work but for me it really muddies the meaning/invariant of 'base' and 'newdata'. Anyway, I just copy/pasted all this from Attributes. Seems to me like the idea is to do all the copying in one place, in build(). Does that change anything for you? Should we consider your proposed change for Attributes too?
Fully remove Args.Extensions from the public API (@internal) Make TODO non-javadoc google-java-format unused imports
👍 I can't take credit though, just copy pasted from Attributes. LMK if you think one of the other special-purpose containers would be a better fit. |
Update NameResolverTest. Include setAll() coverage
grpc-binder's upcoming
IntentNameResolverneeds to know the target Android user so it can resolve target URIs in the correct place. Unfortunately, Android's built in intent:// URI scheme has no way to specify a user and in fact theandroid.os.UserHandleobject can't reasonably be encoded as a String at all.We solve this problem by extending
NameResolver.Argsto permit externally defined arguments using the same type-safe and domain-specificKey<T>pattern used byCallOptions,ContextandCreateSubchannelArgs. New "extension" arguments could apply to all NameResolvers of a certain URI scheme, to all NameResolvers producing a particular type ofjava.net.SocketAddress, or even to a specificNameResolversubclass.A follow up PR will move the target
UserHandlefrom being a property of theManagedChannelto being a property of theSocketAddress.