Remove channelz from LB policy API.

[markdroth]

This also significantly simplifies the channelz node creation code. And it addresses the lock-inversion failure that is preventing #18496 from being merged.

---

This change is 

[markdroth]

This is now ready for review.

[markdroth]

This is now based on #19202.

[markdroth]

It looks like I've accidentally introduced a deadlock in the channelz code. I've run out of steam for today, but I'll work on this more on Monday.

[markdroth]

This is now based on #19244 instead of #19202.

I think this is once again ready for review.

[markdroth]

The dependent PR has been merged, so the only changes here are now the ones from this PR itself.

[AspirinSJL]

I will review this PR today.

[markdroth]

Thanks!

[markdroth]

@veblush, you may want to review the changes to the channelz API here.

[AspirinSJL]

Reviewed 12 of 23 files at r1, 3 of 17 files at r2, 7 of 16 files at r5.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @apolcyn, @markdroth, and @veblush)

---

src/core/ext/filters/client_channel/lb_policy.h, line 207 at r6 (raw file):

const char* message

Might be clearer to state the ownership in the comment.

---

src/core/ext/filters/client_channel/resolving_lb_policy.cc, line 538 at r6 (raw file):

    service_config_error_string = nullptr;
  } else {
    gpr_free(service_config_error_string);

If it's null, we don't need to free it?

---

src/core/ext/filters/client_channel/subchannel_interface.h, line 100 at r6 (raw file):

  // TODO(roth): This method should be removed as part of moving the
  // backoff code from subchannel to LB policy.

Wait, we are keeping backoff in subchannel, right?

---

src/core/lib/channel/channelz.h, line 194 at r6 (raw file):

First bit

It's "Least significant bit"?

---

src/core/lib/channel/channelz.cc, line 227 at r6 (raw file):

                           GRPC_JSON_STRING, false);
  }
  json = data;

Why should we add these two lines?

---

src/core/lib/channel/channelz.cc, line 250 at r6 (raw file):

  MutexLock lock(&child_mu_);
  grpc_json* json_iterator = nullptr;

We can swap these two lines.

---

src/core/lib/surface/channel.cc, line 201 at r6 (raw file):

      grpc_core::channelz::GetParentUuidFromArgs(*args);
  // Create the channelz node.
  // We only need to do this for clients here. For servers, this will be

This comment makes more sense at line 258.

---

src/core/lib/surface/channel.cc, line 222 at r6 (raw file):

the factory 

We are only removing the parent uuid?

---

src/core/lib/surface/channel.cc, line 227 at r6 (raw file):

args_to_remove

Prefer InlinedVector<>? If multiple args should be removed.

[markdroth]

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

---

src/core/ext/filters/client_channel/lb_policy.h, line 207 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

const char* message

Might be clearer to state the ownership in the comment.

Done.

---

src/core/ext/filters/client_channel/resolving_lb_policy.cc, line 538 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

If it's null, we don't need to free it?

Done.

---

src/core/ext/filters/client_channel/subchannel_interface.h, line 100 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Wait, we are keeping backoff in subchannel, right?

Yup. I wrote this before we made that decision.

Updated the comment to document the method's behavior.

---

src/core/lib/channel/channelz.h, line 194 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

First bit

It's "Least significant bit"?

Done.

---

src/core/lib/channel/channelz.cc, line 227 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Why should we add these two lines?

It looks like json_iterator isn't used after this, so that doesn't need to be reset. But json does, because it will have been reset in the block above, and we need to reset it here to make subsequent changes.

(I know this is confusing. Our JSON API is really awful.)

---

src/core/lib/channel/channelz.cc, line 250 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

  MutexLock lock(&child_mu_);
  grpc_json* json_iterator = nullptr;

We can swap these two lines.

We could, but I doubt it will make much difference in terms of performance. It seems clearer to put the lock at the top, to make it clear that the whole function executes under the lock.

---

src/core/lib/surface/channel.cc, line 201 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

This comment makes more sense at line 258.

Good point. Done.

---

src/core/lib/surface/channel.cc, line 222 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

the factory 

We are only removing the parent uuid?

Done.

---

src/core/lib/surface/channel.cc, line 227 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

args_to_remove

Prefer InlinedVector<>? If multiple args should be removed.

In this case, there's just one, so a simple array seems fine.

[AspirinSJL]

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

---

src/core/ext/filters/client_channel/lb_policy.h, line 207 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Done.

In the implementation in client_channel.cc, I notice that the message is freed. Does that actually mean this function take ownership of the message?

---

src/core/lib/channel/channelz.cc, line 227 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

It looks like json_iterator isn't used after this, so that doesn't need to be reset. But json does, because it will have been reset in the block above, and we need to reset it here to make subsequent changes.

(I know this is confusing. Our JSON API is really awful.)

Then json = data; can be moved into the block above, because it's not touched if the block is not entered.

---

src/core/lib/surface/channel.cc, line 227 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

In this case, there's just one, so a simple array seems fine.

So the general rule is that if some args are optional to add/remove, we use InlinedVector<>?

[markdroth]

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

---

src/core/ext/filters/client_channel/lb_policy.h, line 207 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

In the implementation in client_channel.cc, I notice that the message is freed. Does that actually mean this function take ownership of the message?

Doh! You're right.

I've changed this to use UniquePtr<char>, so that it's self-documenting.

I also noticed that we were failing to override this method in a few places (which is unfortunately not caught by the compiler due to the way we define GRPC_ABSTRACT), so I've fixed that as well.

---

src/core/lib/channel/channelz.cc, line 227 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Then json = data; can be moved into the block above, because it's not touched if the block is not entered.

Done.

---

src/core/lib/surface/channel.cc, line 227 at r6 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

So the general rule is that if some args are optional to add/remove, we use InlinedVector<>?

The general rule is to do whatever's the most readable. :) In this particular situation, it does seem easier to read to define the array than to declare an InlinedVector<> and then add each element to it separately.

[markdroth]

Reviewable status: 15 of 22 files reviewed, 2 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @veblush)

---

src/core/ext/filters/client_channel/lb_policy.h, line 207 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Doh! You're right.

I've changed this to use UniquePtr<char>, so that it's self-documenting.

I also noticed that we were failing to override this method in a few places (which is unfortunately not caught by the compiler due to the way we define GRPC_ABSTRACT), so I've fixed that as well.

Actually, I changed my mind -- it really doesn't make sense for this method to take ownership, so I've changed it back to const char* and made it the caller's responsibility to free it.

[AspirinSJL]

Reviewed 1 of 7 files at r8, 6 of 6 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn and @veblush)

[markdroth]

Known issues: #18416 #17007