Move name resolution retry from managed channel to name resolver.

[temawi]

This change has these main aspects to it:

1. Removal of any name resolution responsibility from ManagedChannelImpl
2. Creation of a new RetryScheduler to own generic retry logic
   • Can also be used outside the name resolution context
3. Creation of a new RetryingNameScheduler that can be used to wrap any polling name resolver to add retry capability
4. A new facility in NameResolver to allow implementations to notify listeners on the success of name resolution attempts
   • RetryingNameScheduler relies on this

[larry-safran]

Checkpointing review will continue tomorrow.

[larry-safran]

Should make sure that in release notes this change is mentioned since it breaks source-code compatibility.

[temawi]

For sure, this will be highlighted.

[zhangkun83]

@ejona86 are we now not trying to provide a smoother transition?

[ejona86]

The strategy I had spoken to Terry about shouldn't have needed to change this boolean. In fact, the boolean right now is rather broken from a current threading and future threading perspective. Other parts of this change aren't quite right either. We should roll back.

[ejona86]

Reverting in #9767

[larry-safran]

You might want a check to verify that it returns a positive number.

[temawi]

I guess I could, but if the task can't be scheduled for some reason we would get a RejectedExecutionException which is pretty good at conveying what wen't wrong. Since the existing code to schedule retries didn't make this upfront check either, I'm thinking it's ok the way it is. Let me know if you have a stronger opinion on it.

[larry-safran]

Why aren't you using the RetryingNameResolver for the shutdown? Seems like getting the delegate makes mistakes much easier.

[temawi]

Good point, that would be way more precise. I updated all the tests to only use DnsNameResolver when they specifically need its API.

[temawi]

PR passed internal interop tests: https://fusion2.corp.google.com/ci/kokoro/prod:grpc%2Fjava%2Fmaster%2Fbranch%2Fxds_k8s_lb/activity/288eb952-6eb8-4073-a768-73e17deaee4d/invocations/288eb952-6eb8-4073-a768-73e17deaee4d/targets/grpc%2Fjava%2Fmaster%2Fbranch%2Fxds_k8s_lb/log

[ejona86]

The boolean onResult() return will be wrong/delayed or thread-unsafe. There's two possibilities:

• The NR is calling onResult from sync context. In this case lastAddressesAccepted won't be updated in-line in onResult() so the return value will be the previous call's value. Long-term, we probably need onResult to be called from sync context
• The NR is not calling onResult from sync context. In this case lastAddressesAccepted may still be out-dated, as the runnable can run on other threads. But even if it has been run already lastAddressesAccepted is read outside of the sync context and isn't volatile.

I had seen that problem earlier and had suggested to Terry to avoid it by doing the callback. It seems the approach wasn't entirely clear. What I had imagined had no new public API, but would have some private hackery between RetryingNameResolver and ManagedChannelImpl until the point we can fix the threading issues.

Approach:

1. ManagedChannelImpl can't return a boolean, so use a callback instead. To avoid adding temporary API we'd delete once the return is fixed, pass the callback as an Attribute in ResolutionResult. ManagedChannelImpl and RetryingNameResolver would both have visibility to the Attributes.Key
2. RetryingNameResolver should implement start(Listener2) and wrap the Listener2 with its own Listener. Within onResult(), it would modify the ResolutionResult to add the Attributes.Key for the callback before calling the original onResult(). (Note that there's some annoyance here as start(Listener) will need an implementation copied from the base class to delegate to start(Listener2) since it is overridden in ForwardingNameResolver.)