[EventEngine] Simplify EventEngine::DNSResolver API
#33459
Conversation
test/core/ext/filters/event_engine_client_channel_resolver/resolver_fuzzer.cc
Show resolved
Hide resolved
| /// with a status indicating the success or failure of the lookup. | ||
| /// Implementations should pass the appropriate statuses to the callback. | ||
| /// For example, callbacks might expect to receive DEADLINE_EXCEEDED or | ||
| /// When the lookup is complete or cancelled, the \a on_resolve callback |
There was a problem hiding this comment.
Not sure if it's assumed already, but maybe add a note that cancellation is "best effort", in the sense that a lookup will not necessarily fail if it's cancelled (for example a cancellation might race with completion of a successful lookup, in which case the lookup succeeds).
There was a problem hiding this comment.
I'm on the fence on this. Since we don't have an explicit Cancel API now, adding the note here seems a bit out of context. I added the following note to the DNSResolver class declaration:
/// Note: in the gRPC implementation, cancellation (through destruction) is
/// "best effort" as if the request has completed during the cancellation, the
/// result might still be reported through its callback.
PTAL. Thanks!
There was a problem hiding this comment.
I've provided a suggestion for this above.
| } | ||
| // Even if cancellation fails here, OnResolvedLocked will return early, and |
There was a problem hiding this comment.
since the callback is now always invoked, does this callback still apply?
There was a problem hiding this comment.
Yes, I think it still apply. This part of the code hasn't been changed: https://github.com/grpc/grpc/blob/master/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc#L475
And I think it's still the case that the resolver will not see a complete result after ee_client_channel_resolver is Orphaned.
There was a problem hiding this comment.
Got it , thanks (I was thinking about the DNSResolver cancellation rather than "client channel DNS resolver cancellation"
...ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc
Outdated
Show resolved
Hide resolved
copybara-service
bot
added
the
imported
This change simplifies `EventEngine::DNSResolver`'s API based on the proposal: [go/event-engine-dns-resolver-api-changes](http://go/event-engine-dns-resolver-api-changes). Note that this API change + the implementation described in [go/event-engine-dns-resolver-implementation](http://go/event-engine-dns-resolver-implementation) has already been tested against our main test suites and are passing them. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
This change simplifies
EventEngine::DNSResolver's API based on the proposal: go/event-engine-dns-resolver-api-changes. Note that this API change + the implementation described in go/event-engine-dns-resolver-implementation has already been tested against our main test suites and are passing them.