Still have some questions, but the initial frame is up. Appreciate your comments! Thank you!

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @markdroth)

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/xds/xds_api.h, line 91 at r1 (raw file):

      // Fields used for type HEADER.
      std::string header_name;
      // TODO(donnadionne): double check this is correct

I see we should make this std::unique_ptr regex so that we don't have to construct the RE2 each time; i got a "'unique_ptr' has been explicitly marked deleted here" error; I think I fixed it once before, I'll fix it tomorrow

This looks like a great start!

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

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @donnadionne)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.h, line 25 at r1 (raw file):

extern const char* kXdsClusterAttribute;
extern const char* kXdsRequestHashAttribute;

This should probably be defined in a non-xDS-specific way, since we want the ring_hash policy to be able to be used without xDS.

In principle, we want this to be LB-policy-agnostic, since there could be other hash-based LB policies in the future. But for now, ring_hash will be our only one, so for now let's just stick this in client_channel/lb_policy/ring_hash/ring_hash.h.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 39 at r1 (raw file):

const char* kXdsClusterAttribute = "xds_cluster_name";
const char* kXdsRequestHashAttribute = "xds_request_hash";

Similar to above, let's move this to ring_hash.cc.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 419 at r1 (raw file):

    grpc_metadata_batch* initial_metadata) {
  GPR_ASSERT(policy.type == XdsApi::Route::HashPolicy::HEADER);
  absl::optional<uint64_t> new_hash;

No need for this temporary variable. In the places where we compute the hash, we can return it directly. And if we fall through to the end of the function, we can return absl::nullopt.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 422 at r1 (raw file):

  std::string concatenated_value;
  absl::optional<absl::string_view> value;
  value = GetMetadataValue(policy.header_name, initial_metadata,

This can be combined with the previous line.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 427 at r1 (raw file):

    if (!policy.regex.empty()) {
      RE2::Options options;
      options.set_case_sensitive(true);

This is the default, so there's no need to set it explicitly.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 428 at r1 (raw file):

      RE2::Options options;
      options.set_case_sensitive(true);
      auto regex_matcher = absl::make_unique<RE2>(policy.regex, options);

It's extremely inefficient to do this here. We want to compile the regex once at config-validation time, not once per RPC.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 432 at r1 (raw file):

        gpr_log(GPR_DEBUG, "Invalid regex string specified in header hash.");
      } else {
        std::string key;

This needs to be initialized to *value, or else we're not passing the header value into RE2::Replace().

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 433 at r1 (raw file):

      } else {
        std::string key;
        RE2::Replace(&key, *regex_matcher, policy.regex_substitution);

This should use RE2::GlobalReplace().

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 512 at r1 (raw file):

          new_hash = HeaderHashHelper(hash_policy, args.initial_metadata);
          break;
        case XdsApi::Route::HashPolicy::SOURCE_IP:

We decided not to support this, so we can remove this case.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 523 at r1 (raw file):

          GPR_ASSERT(0);
      }
      if (new_hash) {

new_hash.has_value()

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 527 at r1 (raw file):

        // each other out and preserves all of the entropy
        const uint64_t old_value =
            hash ? ((hash.value() << 1) | (hash.value() >> 63)) : 0;

hash.has_value()

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 532 at r1 (raw file):

      // If the policy is a terminal policy and a hash has been generated,
      // ignore the rest of the hash policies.
      if (hash_policy.terminal && hash) {

hash.has_value()

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 548 at r1 (raw file):

    call_config.call_attributes[kXdsClusterAttribute] = it->first;
    call_config.call_attributes[kXdsRequestHashAttribute] =
        absl::StrFormat("%" PRIu64 "", hash.value());

No need for the "".

src/core/ext/xds/xds_api.h, line 85 at r1 (raw file):

    };

    struct HashPolicy {

This struct will need to provide operator==() and ToString() methods.

src/core/ext/xds/xds_api.h, line 86 at r1 (raw file):

    struct HashPolicy {
      enum Type { HEADER, SOURCE_IP, CHANNEL_ID };

As per the comment thread in the design doc, we're not actually going to support SOURCE_IP, so you can remove that from here.

src/core/ext/xds/xds_api.h, line 91 at r1 (raw file):

I'm not sure what the problem was, but it looks like you've changed this to store the regex as a string instead of as an RE2 object. However, as I've mentioned elsewhere, I don't think that's what we want here, because (a) we need to compile the regex anyway as part of config validation, and there's no point in throwing it away and then recompiling it again later, and (b) we don't actually want the overhead of recompiling it for each RPC. So I think we do want to store the compiled regex here.

src/core/ext/xds/xds_api.cc, line 1155 at r1 (raw file):

    }
  }
  // Get HashPolicy from RouteAction

This code needs to be guarded by XdsRingHashEnabled().

src/core/ext/xds/xds_api.cc, line 1172 at r1 (raw file):

          envoy_config_route_v3_RouteAction_HashPolicy_Header_header_name(
              header));
      if (envoy_config_route_v3_RouteAction_HashPolicy_Header_has_regex_rewrite(

Instead of calling this, just check whether the result of envoy_config_route_v3_RouteAction_HashPolicy_Header_regex_rewrite() is null below.

Same thing throughout -- in general, it's unnecessarily repetetive to call the has_foo() method then call the foo() method immediately afterwards; instead, we can just call foo() and check if the result is null.

src/core/ext/xds/xds_api.cc, line 1183 at r1 (raw file):

              envoy_type_matcher_v3_RegexMatchAndSubstitute_pattern(
                  regex_rewrite);
          policy.regex = UpbStringToStdString(

We need to actually construct the RE2 object here, because if the regex is invalid and won't compile, then we need to NACK the config.

src/core/ext/xds/xds_api.cc, line 1197 at r1 (raw file):

      }
    } else if (
        envoy_config_route_v3_RouteAction_HashPolicy_has_connection_properties(

This case can go away since we decided not to support SOURCE_IP anymore.

Took care of code review comments.

My xxhash module import PR is almost ready (just the last single digit build failures), I will merge the 2 together to try out new xxhash code once this one is in good shape.

Thank you for your help!

Reviewable status: 1 of 4 files reviewed, 21 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.h, line 25 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 39 at r1 (raw file):

nst char* kXdsRequestHashAttribute = "xds_request_hash";

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 39 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 419 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 422 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 427 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 428 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 432 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 433 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 512 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 523 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 527 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 532 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 548 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 85 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 86 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 91 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1155 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1172 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1183 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1197 at r1 (raw file):

Done.

This looks pretty good! My comments are mostly cosmetic.

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

Reviewed 21 of 21 files at r3.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @donnadionne)

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h, line 1 at r3 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h, line 29 at r3 (raw file):

}  // namespace grpc_core

#endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_RING_HASH_RING_HASH_H \

Same here.

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc, line 1 at r3 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 39 at r1 (raw file):

I assume this comment was created by accident.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 425 at r3 (raw file):

    if (policy.regex != nullptr) {
      std::string key(*value);
      RE2::GlobalReplace(&key, *(policy.regex), policy.regex_substitution);

No need for parens around policy.regex.

src/core/ext/xds/xds_api.h, line 96 at r3 (raw file):

      HashPolicy() {}
      HashPolicy(const HashPolicy& other) {

Please move the definitions of these methods into the .cc file.

src/core/ext/xds/xds_api.h, line 96 at r3 (raw file):

      HashPolicy() {}
      HashPolicy(const HashPolicy& other) {

Please also define a move ctor and move-assignment operator.

src/core/ext/xds/xds_api.h, line 97 at r3 (raw file):

      HashPolicy() {}
      HashPolicy(const HashPolicy& other) {
        type = other.type;

All fields except regex can be set via an initializer list.

src/core/ext/xds/xds_api.h, line 123 at r3 (raw file):

            }
          } else {
            return (header_name == other.header_name &&

No need for outer parens here.

src/core/ext/xds/xds_api.cc, line 158 at r3 (raw file):

}

std::string XdsApi::Route::HashPolicy::ToString() const {

Please move this up to line 140, with its own heading-style comment for XdsApi::Route::HashPolicy. When you move the other methods from the .h file, they can go under that same heading.

src/core/ext/xds/xds_api.cc, line 175 at r3 (raw file):

        (regex == nullptr) ? "" : regex->pattern(), regex_substitution));
  }
  return absl::StrJoin(contents, "\n");

Let's use something like absl::StrCat('{', absl::StrJoin(contents, ", "), '}').

src/core/ext/xds/xds_api.cc, line 186 at r3 (raw file):

  contents.push_back(matchers.ToString());
  for (const HashPolicy& hash_policy : hash_policies) {
    contents.push_back(hash_policy.ToString());

StrCat("hash_policy=", hash_policy.ToString())

src/core/ext/xds/xds_api.cc, line 1189 at r3 (raw file):

      policy.terminal =
          envoy_config_route_v3_RouteAction_HashPolicy_terminal(hash_policy);
      const envoy_config_route_v3_RouteAction_HashPolicy_Header* header =

Instead of doing both of these up-front, I suggest something like this:

if ((const auto* header = envoy_config_route_v3_RouteAction_HashPolicy_header(hash_policy)) != nullptr) {
  // ...handle header...
} else if ((const auto* filter_state = envoy_config_route_v3_RouteAction_HashPolicy_filter_state(hash_policy)) != nullptr) {
  // ...handle filter_state...
}

src/core/ext/xds/xds_api.cc, line 1208 at r3 (raw file):

              envoy_type_matcher_v3_RegexMatchAndSubstitute_pattern(
                  regex_rewrite);
          if (regex_matcher != nullptr) {

Let's reverse this condition, so that the error-handling code is right after the condition that detects it.

In other words, instead of saying if (not error) { handle success } else { handle error }, please say if (error) { handle error } handle success.

src/core/ext/xds/xds_api.cc, line 1217 at r3 (raw file):

              gpr_log(
                  GPR_DEBUG,
                  "RouteAction HashPolicy contains policy specifier Head with "

s/Head/Header/

Addressed all the comments. If this is all good, I will move this with the xxhash PR and try out xxhash calls and then submit together.

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @donnadionne and @markdroth)

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h, line 1 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h, line 29 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc, line 1 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 39 at r1 (raw file):

yes

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 425 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 96 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 96 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 97 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 123 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 158 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 175 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 186 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1189 at r3 (raw file):

couldn't quite do declare, assign, and compare all in if.

C++17 will allow this but using a ";" syntax: https://www.tutorialspoint.com/cplusplus17-if-statement-with-initializer

src/core/ext/xds/xds_api.cc, line 1208 at r3 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1217 at r3 (raw file):

Done.

This looks really good! Just a couple of very minor bugs remaining.

Maybe merge the xxhash PR first, and then this one separately? That will probably make it easier to understand both changes.

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

Reviewed 5 of 5 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @donnadionne)

src/core/ext/xds/xds_api.cc, line 1189 at r3 (raw file):

Oh, cool -- something to look forward to in C++17!

src/core/ext/xds/xds_api.cc, line 170 at r4 (raw file):

      header_name(std::move(other.header_name)),
      regex_substitution(std::move(other.regex_substitution)) {
  if (other.regex != nullptr) {

No need for this check -- even if it's null, you can just move it unconditionally. This can be done in the initializer list with the other fields.

src/core/ext/xds/xds_api.cc, line 179 at r4 (raw file):

  type = other.type;
  header_name = std::move(other.header_name);
  if (other.regex != nullptr) {

No need to check this, just move it unconditionally. (In fact, in this case, checking is actually wrong, because if the current object (*this) already has this field populated, we'd fail to clear it by not doing this move.)

src/core/ext/xds/xds_api.cc, line 196 at r4 (raw file):

    } else {
      return header_name == other.header_name &&
             regex->pattern() == other.regex->pattern() &&

There's no guarantee here that other.regex is not null.

I think before this statement, you need to add:

if (other.regex == nullptr) return false;

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

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 396 at r4 (raw file):

  // they are not visible to the LB policy in grpc-go.
  if (absl::EndsWith(header_matcher.name(), "-bin") ||
      header_matcher.name() == "grpc-previous-rpc-attempts") {

Note that we no longer need this special case for "grpc-previous-rpc-attempts", since we've now moved the routing code from the LB policy (which was below retries) to the ConfigSelector (which is above retries).

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 420 at r4 (raw file):

  GPR_ASSERT(policy.type == XdsApi::Route::HashPolicy::HEADER);
  std::string concatenated_value;
  absl::optional<absl::string_view> value = GetMetadataValue(

This should have the same special cases that we have for headers in route matching (see HeaderMatchHelper() above): ignore headers with "-bin" suffixes, and special-case the value for "content-type".

fixed

Reviewable status: 2 of 20 files reviewed, 8 unresolved discussions (waiting on @markdroth and @voidzcy)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 396 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 420 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 425 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 502 at r4 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 640 at r5 (raw file):

        case XdsApi::Route::HashPolicy::CHANNEL_ID:
          key = static_cast<uint64_t>(reinterpret_cast<uintptr_t>(resolver));
          // TODO(donnadionne): new_hash = xx_hash(key).

I should just be using the pointer value as the hash, correct? no need to call hash function here?

src/core/ext/xds/xds_api.cc, line 170 at r4 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 179 at r4 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 196 at r4 (raw file):

Done.

This looks pretty good! Remaining comments are minor.

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

Reviewed 19 of 19 files at r6.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @donnadionne and @voidzcy)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 420 at r4 (raw file):

Instead of duplicating the code from HeaderMatchHelper, how about refactoring into a common helper function that can be used in both places?

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 640 at r5 (raw file):

Good question... I think this should probably be okay, at least for now. There are some potential problems if ASLR is not enabled (which can apparently happen on Windows in some cases) or on 32-bit machines, but let's not worry about those cases until we see a problem.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 562 at r6 (raw file):

    return absl::nullopt;
  } else if (policy.header_name == "content-type") {
    std::string key("application/grpc");

This does an unnecessary allocation. Instead, please use absl::string_view or just const char*.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 563 at r6 (raw file):

  } else if (policy.header_name == "content-type") {
    std::string key("application/grpc");
    return XXH64(key.c_str(), key.size(), 0);

We still need to apply the regex code below in this case.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @markdroth and @voidzcy)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 420 at r4 (raw file):

sure

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 562 at r6 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 563 at r6 (raw file):

Done.

Just a few minor comments remaining, but one of them is a use-after-free bug.

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

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @donnadionne and @voidzcy)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 44 at r7 (raw file):

const char* kXdsClusterAttribute = "xds_cluster_name";
const char* kHeaderContentType = "application/grpc";

I don't think this requires a global constant. It's fine to declare this inline inside of the one function where it's actually used, just like it was before.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 529 at r7 (raw file):

}

absl::optional<absl::string_view> HeaderMatchHelper(

Please call this function GetHeaderValue().

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 530 at r7 (raw file):

absl::optional<absl::string_view> HeaderMatchHelper(
    const std::string& header_matcher_name,

This parameter should be of type absl::string_view.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 530 at r7 (raw file):

absl::optional<absl::string_view> HeaderMatchHelper(
    const std::string& header_matcher_name,

Please call this parameter header_name.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 531 at r7 (raw file):

absl::optional<absl::string_view> HeaderMatchHelper(
    const std::string& header_matcher_name,
    grpc_metadata_batch* initial_metadata, std::string* concatenated_value) {

Please make the initial_metadata argument the first argument, since this function is now operating primarily on that object.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 560 at r7 (raw file):

  GPR_ASSERT(policy.type == XdsApi::Route::HashPolicy::HEADER);
  std::string concatenated_value;
  absl::optional<absl::string_view> matcher = HeaderMatchHelper(

Please call this variable header_value.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 566 at r7 (raw file):

    size_t size = matcher->size();
    if (policy.regex != nullptr) {
      std::string modified_key(matcher->data(), matcher->size());

This string object is declared inside of this scope, and then we are setting key to point to the data inside of this object, and then we're using key after this object goes out of scope. This is a use-after-free bug.

Instead, I suggest writing this as follows:

std::string value_buffer;
absl::optional<absl::string_view> header_value = GetHeaderValue(
    initial_metadata, policy.header_name, &value_buffer);
if (policy.regex != nullptr) {
  // If GetHeaderValue() did not already store the value in
  // value_buffer, copy it there now, so we can modify it.
  if (header_value->data() != value_buffer.data()) {
    value_buffer = std::string(*header_value);
  }
  RE2::GlobalReplace(&value_buffer, *policy.regex,
                     policy.regex_substitution);
  header_value = value_buffer;
}
return XXH64(header_value.data(), header_value.size(), 0);

Reviewable status: 20 of 21 files reviewed, 9 unresolved discussions (waiting on @markdroth and @voidzcy)

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 44 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 529 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 530 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 530 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 531 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 560 at r7 (raw file):

Done.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 566 at r7 (raw file):

Done.

Looks great!

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @voidzcy)