Add MetadataInterface abstraction to LB policy API.

[markdroth]

This change is 

[markdroth]

I still need to figure out why the Bazel RBE MSAN tests are failing to build, but otherwise, this is ready for review.

@soheilhy, I'd like your feedback on this change w.r.t. the way I'm using the metadata API in the grpclb LB policy. Since the whole point of this change is to provide a cleaner API for LB policies to access metadata, they're not going to have the full power of our internal metadata API, so I suspect that there will be some performance degredation here. I am willing to accept some performance degredation in the grpclb case, since our plan is to deprecate grpclb in favor of xds, but I do want to try to provide a metadata API here that is reasonably efficient for the general case. Any suggestions you can offer about a better way to structure the API to acheive that while still hiding the details of our internal metadata APIs would be appreciated.

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

[markdroth]

@arjunroy, I'd like your feedback on this API as well, for the same reason as I mentioned for Soheil above. Thanks!

[soheilhy]

Thank you for including me!

[markdroth]

The iterator API issue you pointed out is definitely a problem -- thanks for catching that! I'll need to think about how to restructure to avoid that...

[markdroth]

Soheil, I've addressed most of your comments here, but I still need to work on the issue we discussed via IM about the interned strings. I'll see if I can plumb through a different way to handle the Trailers-Only responses, at which point the metadata key used by grpclb doesn't have to be in the static metadata table.

Please let me know what you think. Thanks!

[soheilhy]

Looks great to me. Thank you!

I look forward to the fix for trailing-only. With that I would expect no measurable performance penalty.

[markdroth]

I just realized that I don't actually have to fix the Trailers-Only problem, because that's only an issue on the server side, but the LB policy is adding metadata on the client side. So I've removed the attempt to intern the key slice and am instead just passing down the static slice.

If needed, I could still add an API for the LB policy to pre-allocate a slice, which it could then pass to Add() on various different calls. But I'm not sure that's really worth doing, since the simple static slice should be sufficient here. What do you think?

[soheilhy]

Thank you @markdroth ! Nice idea and looks great to me. I don't think this would cause any measure able regression and we probably wouldn't need to pre-allocate a slice. @arjunroy WDYT?

FWIW, there is a compile time failure on all tests:

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:1441:27: error: use of undeclared identifier 'GRPC_MDELEM_LB_TOKEN_EMPTY'
  void* lb_token = (void*)GRPC_MDELEM_LB_TOKEN_EMPTY.payload;

[AspirinSJL]

Reviewed 4 of 9 files at r1, 2 of 2 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @apolcyn, @arjunroy, and @markdroth)

---

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

access

accessing

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 450 at r5 (raw file):

// vtables for channel args for LB token and client stats.
void* lb_token_copy(void* token) { return (void*)gpr_strdup((char*)token); }

Please change to static_cast().

[AspirinSJL]

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

[markdroth]

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

---

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

Previously, AspirinSJL (Juanli Shen) wrote…

access

accessing

Done.

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 450 at r5 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Please change to static_cast().

Done.

[markdroth]

Looks like the merge from head caused the build problem. That should be fixed now.

I needed to make one more change here, which was to fix the way the client_load_reporting filter finds the ClientStats object in metadata. Since we're not interning the keys anymore, we don't have a batch callout for the key used to store the ClientStats object, so we need to iterate through the metadata in the filter to find it. This is unfortunate, but given that we're trying to move away from grpclb, I think we can live with it. I have slightly compensated for this by moving the filter closer to the top of the stack, so that there are fewer metadata elements to iterate through (my test shows that there should be 4 of them in a typical config).

PTAL. Thanks!

[markdroth]

One more change: We can no longer use the static metadata key in the server load reporting filter. This is probably another place where performance is going to be worse, since we now need to do a string comparison instead of comparing the static slices by address. But as before, I'm willing to accept this for now, since we're trying to move away from grpclb.

I do wonder if there's a better way we can handle this sort of case, though. Suggestions welcome.

[AspirinSJL]

Reviewed 6 of 8 files at r7, 2 of 2 files at r8.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @apolcyn, @arjunroy, and @markdroth)

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 617 at r8 (raw file):

      client_stats->AddCallStarted();
    }
    // Encode the LB token in metadata.

Why do we need to reorder the two encoding here?

[markdroth]

Reviewable status: 8 of 11 files reviewed, 5 unresolved discussions (waiting on @apolcyn, @arjunroy, @AspirinSJL, and @markdroth)

---

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 617 at r8 (raw file):

Previously, AspirinSJL (Juanli Shen) wrote…

Why do we need to reorder the two encoding here?

It's a slight optimization. The client_load_reporting filter will iterate over the entries in order until it finds the one for the ClientStats object. So the earlier in the list we put that entry, the less work it has to do.

[AspirinSJL]

Reviewed 3 of 3 files at r9.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn, @arjunroy, and @markdroth)

[soheilhy]

Still looks great. If the issue you mentioned flares up in benchmarks, we can try to fix it later.

[markdroth]

Known issues: #19722 #19614 #19422 #18449 #19170