Introduce onResult2 in NameResolver Listener2 that returns Status #11313
Conversation
…the acceptance of the name resolution by the load balancer.
…the acceptance of the name resolution by the load balancer.
| private Status onResult2(final ResolutionResult resolutionResult, boolean useResolutionResultListener) { | ||
| syncContext.throwIfNotInThisSynchronizationContext(); | ||
| if (ManagedChannelImpl.this.nameResolver != resolver) { | ||
| return Status.FAILED_PRECONDITION; |
There was a problem hiding this comment.
As a rule, don't use bare status codes for errors. It is very difficult to find where they came from. Always include at least a description or cause.
Although I think in both of these FAILED_PRECONDITION cases we could just return OK instead. We aren't rejecting the config so much all future updates will be rejected; there's no need for the name resolver to retry.
There was a problem hiding this comment.
Since the check
if (ManagedChannelImpl.this.nameResolver != resolver)
is already made here, do we still need the check
if (NameResolverListener.this.helper == ManagedChannelImpl.this.lbHelper) {
later in the same method, which can never happen since the method runs inside the syncContext?
I added a new unit test noMoreCallbackAfterLoadBalancerShutdown_configError to test that the retry for name resolving doesn't happen after the onResult has a config error. However I'm finding that the test succeeds even if I don't change returning the FAILED_PRECONDITION to OK, and I notice that the scheduling for next retry here happens https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/RetryingNameResolver.java#L107 with 0.9 s delay but it doesn't get triggered even after adding a sleep statement longer than that in the test. Still looking into this.
There was a problem hiding this comment.
I realized the executor is a fake executor service, so I have added the assertion on the fake clock pending task now to ensure that no retry of the name resolution gets scheduled for the case of config error in ResolutionResult.
…en shutdown already.
…doc. Changed "InlineMe" for onAddresses to ues onResult2 instead of onResult also.
…throw UnImplemented exception from onResult2 because we can't replace references to onResult with onResult2 in "InlineMe" for the deprecation annotation of onAddresses.
…atus (grpc#11313)" This reverts commit 9ba2f9d. It causes a channel panic due to unimplemented onResult2(). ``` java.lang.UnsupportedOperationException: Not implemented. at io.grpc.NameResolver$Listener2.onResult2(NameResolver.java:257) at io.grpc.internal.DnsNameResolver$Resolve.lambda$run$0(DnsNameResolver.java:334) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126) at io.grpc.internal.DnsNameResolver$Resolve.run(DnsNameResolver.java:333) ```
…atus (grpc#11313)" This reverts commit 9ba2f9d. It causes a channel panic due to unimplemented onResult2(). ``` java.lang.UnsupportedOperationException: Not implemented. at io.grpc.NameResolver$Listener2.onResult2(NameResolver.java:257) at io.grpc.internal.DnsNameResolver$Resolve.lambda$run$0(DnsNameResolver.java:334) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126) at io.grpc.internal.DnsNameResolver$Resolve.run(DnsNameResolver.java:333) ``` b/356669977
|
Reverting in #11421 |
…atus (#11313)" This reverts commit 9ba2f9d. It causes a channel panic due to unimplemented onResult2(). ``` java.lang.UnsupportedOperationException: Not implemented. at io.grpc.NameResolver$Listener2.onResult2(NameResolver.java:257) at io.grpc.internal.DnsNameResolver$Resolve.lambda$run$0(DnsNameResolver.java:334) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126) at io.grpc.internal.DnsNameResolver$Resolve.run(DnsNameResolver.java:333) ``` b/356669977
…atus (grpc#11313)" This reverts commit 9ba2f9d. It causes a channel panic due to unimplemented onResult2(). ``` java.lang.UnsupportedOperationException: Not implemented. at io.grpc.NameResolver$Listener2.onResult2(NameResolver.java:257) at io.grpc.internal.DnsNameResolver$Resolve.lambda$run$0(DnsNameResolver.java:334) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126) at io.grpc.internal.DnsNameResolver$Resolve.run(DnsNameResolver.java:333) ``` b/356669977
…atus (#11313)" (#11423) This reverts commit 9ba2f9d. It causes a channel panic due to unimplemented onResult2(). ``` java.lang.UnsupportedOperationException: Not implemented. at io.grpc.NameResolver$Listener2.onResult2(NameResolver.java:257) at io.grpc.internal.DnsNameResolver$Resolve.lambda$run$0(DnsNameResolver.java:334) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) at io.grpc.SynchronizationContext.execute(SynchronizationContext.java:126) at io.grpc.internal.DnsNameResolver$Resolve.run(DnsNameResolver.java:333) ``` b/356669977
…tatus (grpc#11313)" This reverts commit ebffb0a.
Lets the Name Resolver receive the status of the acceptance of the name resolution by the load balancer.