This is looking very good! Just a few minor issues left.

Please let me know if you have any questions. Thanks!

Reviewed 27 of 31 files at r6, 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn)

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 601 at r5 (raw file):

Good point about the behavior change. The TODO is fine.

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 580 at r7 (raw file):

  // gets the singleton instance, possibly creating it first
  static AresDNSResolver* GetOrCreate() {
    static AresDNSResolver* instance;

This can just be:

static AresDNSResolver* instance = new AresDNSResolver();
return instance;

Since it's static, the assignment will be evaluated only once.

Same thing in all implementations.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 175 at r5 (raw file):

Using std::move() inside of the lambda is still what we want, or else we're doing two copies instead of just the one that we can't avoid until we migrate to C++14. To make that possible, you just need to use the mutable keyword on the lambda.

[this, addresses_or]() mutable { OnResolvedLocked(std::move(addresses_or)); }

Later, when we can use C++14, this can become:

[this, addresses_or = std::move(addresses_or)]() mutable { OnResolvedLocked(std::move(addresses_or)); }

src/core/lib/iomgr/iomgr_custom.cc, line 23 at r7 (raw file):

#include "src/core/lib/iomgr/iomgr_custom.h"

#include "absl/functional/bind_front.h"

Doesn't look like this is used in this file.

src/core/lib/iomgr/resolve_address_custom.h, line 76 at r5 (raw file):

Instead of GetOrCreate() and set_vtable(), how about something like this:

// Creates the global custom resolver with the specified vtable.
static void CreateCustomResolver(const grpc_custom_resolver_vtable* vtable);

// Gets the global custom resolver.
static CustomDNSResolver* Get();

That way, it's not possible to change vtable after creation, which I think is the semantic we want here.

This will require having the static pointer outside of the Get() method, but I think that's a worthwhile trade-off in this case.

test/core/iomgr/resolve_address_test.cc, line 60 at r7 (raw file):

bool g_resolver_type_configured;

class ResolveAddressTest : public ::testing::TestWithParam<const char*> {

If we're using TestWithParam<>, don't we need an INSTANTIATE_TEST_SUITE_P() call somewhere for ResolveAddressTest, and then use TEST_P() instead of TEST_F() for the individual test cases? Otherwise, I'm not sure these tests are actually being run.

Thanks, I believe I've addressed comments again. PTAL

Reviewable status: 52 of 62 files reviewed, 5 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 580 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 175 at r5 (raw file):

Done.

src/core/lib/iomgr/iomgr_custom.cc, line 23 at r7 (raw file):

Done.

src/core/lib/iomgr/resolve_address_custom.h, line 76 at r5 (raw file):

Done.

test/core/iomgr/resolve_address_test.cc, line 60 at r7 (raw file):

I am seeing these test cases get ran. However, TestWithParam<> might be making things more complicated to inspect than is needed here.

Refactored to use TEST_F on all test cases.

This looks great!

Reviewed 9 of 10 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

test/core/iomgr/resolve_address_test.cc, line 60 at r7 (raw file):

Thanks, that's much more readable!