Refactor EDS and LRS code from xds LB policy into new XdsClient API.

[markdroth]

This change is 

[markdroth]

This is now built on #20406.

[markdroth]

I've reduced this to two commits. The first one is the code from #20406, and the second one contains the new changes in this PR.

[markdroth]

This PR now contains only its own changes.

[AspirinSJL]

I will review it today.

Reviewable status: 0 of 28 files reviewed, all discussions resolved (waiting on @apolcyn and @AspirinSJL)

[markdroth]

Sorry, not sure what I was thinking there -- we will not need that logic now that we're getting the balancer name from the bootstrap file, and that's why I removed it.

[AspirinSJL]

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @apolcyn and @markdroth)

---

BUILD, line 1324 at r1 (raw file):

        "grpc_base",
        "grpc_client_channel",
        "grpc_secure",

This is no longer needed.

---

build.yaml, line 1142 at r1 (raw file):

  - grpc_base
  - grpc_client_channel
  - grpc_secure

This is no longer needed.

---

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 740 at r1 (raw file):

  // watcher holds a ref to us.
  if (xds_client_ != nullptr) {
    xds_client_->CancelEndpointDataWatch(StringView(server_name_),

Maybe reset endpoint_watcher_ to null after cancelling the watch to be clear that it's cancelled.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 21 at r1 (raw file):

#include <grpc/support/port_platform.h>

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

This block can be moved down.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 108 at r1 (raw file):

owing

Not related to this PR, but this should be "owning".

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 550 at r1 (raw file):

GRPC_MILLIS_INF_FUTURE

Why do we remove the deadline?

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 754 at r1 (raw file):

update.drop_config != nullptr

We should remove this.

1. This is always true.
   

   grpc/src/core/ext/filters/client_channel/lb_policy/xds/xds_load_balancer_api.cc

   Line 300 in dbe7f35

   update->drop_config = MakeRefCounted<XdsDropConfig>();

2. Even if this can be false, we should add more checking before we dereferencing both pointers.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 801 at r1 (raw file):

lb_call

This is now call_ or something. Same for LRS.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 808 at r1 (raw file):

  // Ignore status from a stale call.
  if (eds_calld->IsCurrentCallOnChannel()) {
    // Try to restart the call.

Sorry, why don't we re-resolve in this case?

[AspirinSJL]

Reviewed 12 of 12 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @apolcyn and @markdroth)

[markdroth]

Thanks for the review!

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

---

BUILD, line 1324 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

This is no longer needed.

Done.

---

build.yaml, line 1142 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

This is no longer needed.

Done.

---

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 740 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Maybe reset endpoint_watcher_ to null after cancelling the watch to be clear that it's cancelled.

I don't think that's necessary, because endpoint_watcher_ cannot be used for any purpose other than cancelling the watch -- it cannot even be deferenced by the LB policy, because it is not holding a ref to it. And the semantics of cancelling the watch are such that even if we used the same address to cancel the watch twice, the second one would be a no-op.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 21 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

This block can be moved down.

Actually, it looks like these are already included below.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 108 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

owing

Not related to this PR, but this should be "owning".

Done.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 550 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

GRPC_MILLIS_INF_FUTURE

Why do we remove the deadline?

I don't think there was ever actually a good reason to use a deadline. David's original implementation of the grpclb LB policy had the LB call's deadline set to the deadline of the call that triggered the existence of the LB policy, which was clearly the wrong thing to do. When we realized this, he fixed it by making it an independent channel arg, but in hindsight I think we should have just removed the deadline altogether, because no user has ever asked for this, and I can't think of a good reason to allow this to be tunable.

Prior to this PR, the xds code was using the same parameter name as the grpclb policy (GRPC_ARG_GRPCLB_CALL_TIMEOUT_MS), and it really doesn't make sense to use a grpclb-specific parameter in the xds code, so I just removed support for it.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 754 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

update.drop_config != nullptr

We should remove this.

1. This is always true.
   

   grpc/src/core/ext/filters/client_channel/lb_policy/xds/xds_load_balancer_api.cc

   Line 300 in dbe7f35

   update->drop_config = MakeRefCounted<XdsDropConfig>();

2. Even if this can be false, we should add more checking before we dereferencing both pointers.

Done.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 801 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

lb_call

This is now call_ or something. Same for LRS.

Done.

---

src/core/ext/filters/client_channel/xds/xds_client.cc, line 808 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Sorry, why don't we re-resolve in this case?

We do, but re-resolution is handled in the LB policy, not in the xds client. The code here calls the xds LB policy's EndpointWatcher::OnError(), and that code decides whether to re-resolve.

[AspirinSJL]

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @markdroth)

---

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 740 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think that's necessary, because endpoint_watcher_ cannot be used for any purpose other than cancelling the watch -- it cannot even be deferenced by the LB policy, because it is not holding a ref to it. And the semantics of cancelling the watch are such that even if we used the same address to cancel the watch twice, the second one would be a no-op.

Then it deserves a comment that describes how it should be used.

[markdroth]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @AspirinSJL)

---

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc, line 740 at r1 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Then it deserves a comment that describes how it should be used.

Done.

[AspirinSJL]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

[markdroth]

Known issues: #19422 #20308 #20471