This looks really good!

I have a lot of comments here, but that's mostly a reflection of the invasiveness of this change; most of the comments are fairly minor things. The only structural issue is the way the health checking config is being handled, which I've noted in a few places.

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

Reviewed 47 of 48 files at r1, 27 of 27 files at r2.
Reviewable status: all files reviewed, 60 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @yashykt)

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

      void* arg, Resolver::Result* result, const char** lb_policy_name,
      const ParsedLoadBalancingConfig** lb_policy_config,
      const HealthCheckParsedObject** health_check);

This should not need to be added to this API. Instead, I think we can do this by simply changing ChannelData::ClientChannelControlHelper::CreateSubchannel() to read the health check service name from chand_->service_config_ and pass it into the subchannel as a channel arg.

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

  // Data from service config.
  bool received_service_config_data_ = false;
  grpc_core::RefCountedPtr<ServerRetryThrottleData> retry_throttle_data_;

No need for the grpc_core:: prefix.

Same thing throughout.

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

  if (chand->service_config() != nullptr) {
    service_config_ = chand->service_config();

Each call to chand->service_config() increments the ref-count to return a new RefCountedPtr<>. So what we're doing here is incrementing the refcount, decrementing it, and then re-incrementing it.

We should probably rewrite this as:

service_config_ = chand->service_config();
if (service_config_ != nullptr) {

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

                    client_channel_service_config_parser_index()])
              .get());
      call_context_[GRPC_SERVICE_CONFIG_METHOD_PARAMS].value =

Instead of having two separate call context elements, how about defining a ServiceConfigCallData class that holds a ref to the ServiceConfig object and contains a pointer to the method params vector? This would avoid declaring two call context elements, and it would make it more obvious to the caller that the two pieces of data share the same lifetime semantics.

The ServiceConfigCallData class could also provide convenient methods that get the right element from the vector for a given parser, for both global params and per-method params.

src/core/ext/filters/client_channel/client_channel_factory.h, line 39 at r1 (raw file):

  virtual Subchannel* CreateSubchannel(
      const grpc_channel_args* args,
      const HealthCheckParsedObject* health_check) GRPC_ABSTRACT;

I don't think we want to plumb this in here. Doing this would require internal changes when this PR is imported that will just wind up being undone when my PR is ready.

For now, I suggest just passing in the health check service name via a channel arg. I'll undo that change in my PR, but at least it won't cause churn for the imports.

src/core/ext/filters/client_channel/client_channel_plugin.cc, line 56 at r1 (raw file):

void grpc_service_config_register_parsers() {
  grpc_core::internal::ClientChannelServiceConfigParser::Register();
  grpc_core::MessageSizeParser::Register();

The message size filter is a separate plugin from the client_channel code and should be able to be built without this (or vice-versa), so they should be registered independently. The message_size filter should do its own registration (in grpc_message_size_filter_init()); the client_channel filter should not depend on it or reference it in any way.

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

    virtual Subchannel* CreateSubchannel(
        const grpc_channel_args& args,
        const HealthCheckParsedObject* health_check) GRPC_ABSTRACT;

This definitely shouldn't be leaked into the LB policy API -- I'm actively working with Sanjay to try to eliminate gRPC-specific things from this API, and this moves in the opposite direction.

See my comments in client_channel.cc and client_channel_factory.h -- I think that for now, a better solution will be to pass in the health check service name via a channel arg.

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

  struct UpdateArgs {
    ServerAddressList addresses;
    const ParsedLoadBalancingConfig* config = nullptr;

The LB policy may need to hold on to the config for asynchronous use (e.g., in xds, we will need to pass the child policy config to a new child policy whenever the balancer sends us a new locality). To do this, it needs to somehow take a ref to the ServiceConfig class that owns this ParsedLoadBalancingConfig.

Holding this ref was one of the main functions of the Config class that was here before. I suspect that we'll still need something like that here.

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

    ServerAddressList addresses;
    const ParsedLoadBalancingConfig* config = nullptr;
    const HealthCheckParsedObject* health_check = nullptr;

Same as above -- this should not be present in this API.

I'll stop noting this in individual places, but in general, I don't think we should be adding HealthCheckParsedObject to any APIs.

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

  /// Returns the JSON node of policy (with both policy name and config content)
  /// given the JSON node of a LoadBalancingConfig array.
  static grpc_json* ParseLoadBalancingConfig(const grpc_json* lb_config_array,

Let's move this to be an internal function in lb_policy_registry.cc. It doesn't actually need to be in this API anymore.

src/core/ext/filters/client_channel/lb_policy_factory.h, line 30 at r1 (raw file):

namespace grpc_core {

class ParsedLoadBalancingConfig {

Please move this to the lb_policy.h file, since that's where people are going to start looking at this API.

Also, please add a comment documenting what it's for.

src/core/ext/filters/client_channel/lb_policy_factory.h, line 34 at r1 (raw file):

  virtual ~ParsedLoadBalancingConfig() = default;

  virtual const char* name() const GRPC_ABSTRACT;

Please document that this method returns the policy name.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 43 at r1 (raw file):

 public:
  struct RetryThrottling {
    int max_milli_tokens = 0;

These fields should be of type intptr_t.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 50 at r1 (raw file):

      UniquePtr<ParsedLoadBalancingConfig> parsed_lb_config,
      const char* parsed_deprecated_lb_policy,
      const grpc_core::Optional<RetryThrottling>& retry_throttling)

No need for grpc_core:: prefix, since we're already in that namespace.

Same thing throughout.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 116 at r1 (raw file):

// A container of processed fields from the resolver result. Simplifies the
// usage of resolver result.
class ProcessedResolverResult {

I suspect that we don't actually need this class anymore. The main function of this class was to do the service config parsing, but that's now done by the registered parsers before the resolver even returns. So by the time we get to client_channel's ChannelData::ParsedLoadBalancingConfig() function (which is what instantiates this class), we can instead just directly use the parsed objects.

To say this another way, I think we should just move the remaining logic from here directly into ChannelData::ProcessResolverResultLocked() (or maybe into a couple of short helper methods on ChannelData that can be called from ProcessResolverResultLocked()).

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 134 at r1 (raw file):

  }

  const HealthCheckParsedObject* health_check() { return health_check_; }

This could just return the health check service name as a string (or null if unset).

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 62 at r1 (raw file):

ProcessedResolverResult::ProcessedResolverResult(
    Resolver::Result* resolver_result, bool parse_retry)

There's not really any point to the parse_retry parameter anymore, since the parsing will happen even if retries are disabled. I'm okay with that little bit of extra work, but the parameter here is now useless, so let's just eliminate it.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 67 at r1 (raw file):

    // call context or metadata.  This would avoid the problem of having
    // to recreate all subchannels whenever the service config changes.
    // It would also avoid the need to pass in the resolver result in

As per this comment, we can change the resolver_result parameter to this function from a pointer to a const reference now. And we can make the same change in ResolvingLoadBalancingPolicy::ProcessResolverResultCallback().

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 113 at r1 (raw file):

          ClientChannelServiceConfigParser::
              client_channel_service_config_parser_index()));

Nit: Please remove blank lines within functions.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 124 at r1 (raw file):

    grpc_uri* uri = grpc_uri_parse(server_uri, true);
    GPR_ASSERT(uri->path[0] != '\0');
    server_name_ = uri->path[0] == '/' ? uri->path + 1 : uri->path;

I don't think server_name_ needs to be a class member anymore. It can be a local variable now.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 127 at r1 (raw file):

    if (parsed_object->retry_throttling().has_value()) {
      retry_throttle_data_ =
          grpc_core::internal::ServerRetryThrottleMap::GetDataForServer(

Suggest moving this call into ChannelData::ServiceConfigSetter::SetServiceConfigData(), so that we're not causing inefficiency by replacing the throttle data before we're actually ready to swap it out. (Doing so will cause an extra level of indirection for each call inside of ServerRetryThrottleData::GetReplacementThrottleDataIfNeeded().)

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 134 at r1 (raw file):

    grpc_uri_destroy(uri);
  }
  if (parsed_object->parsed_lb_config()) {

I think the rest of this function should be combined with ProcessLbPolicyName(), so that all the logic for picking the LB policy name is in one place. (The only reason it was previously split up was that all of the parsing happened here, but that's no longer the case.)

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 147 at r1 (raw file):

    const Resolver::Result& resolver_result) {
  // Prefer the LB policy name found in the service config.
  if (lb_policy_name_ != nullptr) {

This conversion to lower-case should be done only when using the deprecated loadBalancingPolicy field, not when using the new loadBalancingConfig field.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 415 at r1 (raw file):

            "field:retryThrottling error:Duplicate entry"));
      } else {
        grpc_core::Optional<int> max_milli_tokens(false, 0);

No need for grpc_core:: prefix.

Same thing throughout.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 378 at r2 (raw file):

        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "field:loadBalancingPolicy error:Unknown lb policy"));
      } else if (strcmp(field->value, "xds_experimental") == 0) {

This particular check seems a little too special-casey. The only reason I can see that xds can't be supported here is that it has a required parameter in its config, but its config cannot be specified via this field. But there could be other special-cases in the future that require specifying a config, and we're not going to remember to add them all here.

A more general approach would be to have this code invoke the parser of the selected LB policy without a null JSON string and then let the LB policy's parser return an error if it has required parameters.

src/core/ext/filters/client_channel/service_config.h, line 108 at r1 (raw file):

  using ProcessJson = void (*)(const grpc_json*, T*);
  template <typename T>
  void ParseGlobalParams(ProcessJson<T> process_json, T* arg) const;

Can this go away now?

src/core/ext/filters/client_channel/service_config.h, line 117 at r1 (raw file):

  using CreateValue = RefCountedPtr<T> (*)(const grpc_json* method_config_json);
  template <typename T>
  RefCountedPtr<SliceHashTable<RefCountedPtr<T>>> CreateMethodConfigTable(

Can this go away now?

src/core/ext/filters/client_channel/service_config.h, line 127 at r1 (raw file):

  /// Caller does NOT own a reference to the result.
  template <typename T>
  static RefCountedPtr<T> MethodConfigTableLookup(

Can this go away now?

src/core/ext/filters/client_channel/service_config.h, line 179 at r1 (raw file):

  // Returns false on error.
  template <typename T>
  static bool ParseJsonMethodConfig(

Can this go away now?

(Maybe some of the other private methods can go too -- I haven't looked closely at all of them.)

src/core/ext/filters/client_channel/service_config.h, line 60 at r2 (raw file):

/// This is the base class that all service config parsers MUST use to store
/// parsed service config data.
class ServiceConfigParsedObject {

Would it make sense to nest this class inside of the ServiceConfig class? It could be called something like ServiceConfig::ParsedConfig.

src/core/ext/filters/client_channel/service_config.h, line 68 at r2 (raw file):

/// This is the base class that all service config parsers should derive from.
class ServiceConfigParser {

Would it make sense to nest this class inside of the ServiceConfig class? It could be called ServiceConfig::Parser.

src/core/ext/filters/client_channel/service_config.h, line 155 at r2 (raw file):

  // Consumes all the errors in the vector and forms a referencing error from
  // them. If the vector is empty, return GRPC_ERROR_NONE.
  template <size_t N>

Instead of templatizing this, how about defining a type for an error vector that has a particular value here, and then use that type in this method? That way, all callers would use the same value here, and this method would not wind up being duplicated by the compiler if they didn't.

src/core/ext/filters/client_channel/service_config.h, line 156 at r2 (raw file):

  // them. If the vector is empty, return GRPC_ERROR_NONE.
  template <size_t N>
  static grpc_error* CreateErrorFromVector(

Would it make sense to make this a protected method of the ServiceConfigParser class, since it's intended for use in parser subclasses?

If it's needed here in the ServiceConfig class as well, it could be defined as a private method here, and then we can have a protected wrapper method in ServiceConfigParser that calls this. (If you take my suggestion above about making ServiceConfigParser a nested class inside of ServiceConfig, then it will have access to private methods of ServiceConfig.)

This approach would restrict use of this method only to the code that we actually want to call it.

src/core/ext/filters/client_channel/subchannel.cc, line 568 at r1 (raw file):

  grpc_connectivity_state_init(&state_and_health_tracker_, GRPC_CHANNEL_IDLE,
                               "subchannel");

Nit: Please remove blank lines within functions.

src/core/ext/filters/client_channel/health/health_check_parser.h, line 39 at r1 (raw file):

};

class HealthCheckParser : public ServiceConfigParser {

This could just be part of the client_channel parser instead of being a separate one.

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 33 at r1 (raw file):

  const char* service_name = nullptr;
  for (grpc_json* field = json->child; field != nullptr; field = field->next) {
    if (field->key == nullptr) {

This should get caught by the service config parsing framework, right? That's why we're not returning an error here?

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 44 at r1 (raw file):

      for (grpc_json* sub_field = field->child; sub_field != nullptr;
           sub_field = sub_field->next) {
        if (sub_field->key == nullptr) {

Should this be reported as an error?

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 56 at r1 (raw file):

          if (sub_field->type != GRPC_JSON_STRING) {
            *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
                "field:healthCheckConfig error:should be of type string");

This error is not mutually exclusive with the previous one. If they both occur, we should report both at once.

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 65 at r1 (raw file):

  }
  return UniquePtr<ServiceConfigParsedObject>(
      New<HealthCheckParsedObject>(service_name));

We should probably document the lifetime semantics for the parsing API somewhere. In particular, the only reason that this is safe is that the resulting HealthCheckParsedObject will not outlive the JSON tree into which service_name points, because both will be tied to the ServiceConfig object.

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

}

void GrpcLb::ParseLbConfig(const ParsedGrpcLbConfig* grpclb_config) {

This method has almost nothing it in anymore and is only called from a single place. So I suggest removing the method and moving the code to the place where the method is called.

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

    GPR_DEBUG_ASSERT(strcmp(json->key, name()) == 0);
    InlinedVector<grpc_error*, 2> error_list;
    UniquePtr<ParsedLoadBalancingConfig> child_policy = nullptr;

No need for = nullptr here; that's the default value.

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

    InlinedVector<grpc_error*, 2> error_list;
    UniquePtr<ParsedLoadBalancingConfig> child_policy = nullptr;

Nit: Please remove blank lines within functions.

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

  // TODO(yashykt) : does this need to be a gpr_strdup
  balancer_name_ = UniquePtr<char>(gpr_strdup(xds_config->balancer_name()));
  child_policy_config_ = xds_config->child_policy();

See my comment in lb_policy.h about lifetime semantics. What we're doing here is storing a pointer to a field in an object that may no longer exist by the time we actually need to use this field.

I think that what we really need to do here is to pass in a UniquePtr<> of the ParsedXdsConfig, and store that as a data member of the XdsLb class. We can then refer to its fields whenever need to, since we'll own the object.

src/core/ext/filters/message_size/message_size_filter.cc, line 125 at r1 (raw file):

struct channel_data {
  grpc_core::MessageSizeParsedObject::message_size_limits limits;
  grpc_core::RefCountedPtr<grpc_core::ServiceConfig> svc_cfg;

I don't think this field is needed. This filter has no global service config params, only per-method params, so there's no need to store any service config params on a per-channel basis.

src/core/ext/filters/message_size/message_size_filter.cc, line 149 at r1 (raw file):

      limits = static_cast<const grpc_core::MessageSizeParsedObject*>(
          (*objs_vector)[message_size_parser_index].get());
    } else if (chand.svc_cfg != nullptr) {

This else block should not be needed.

src/core/ext/filters/message_size/message_size_filter.cc, line 349 at r1 (raw file):

  GPR_ASSERT(!args->is_last);
  channel_data* chand = static_cast<channel_data*>(elem->channel_data);
  new (chand) channel_data();

If we're going to instantiate this as a C++ object here, then we need to call its dtor in destroy_channel_elem().

src/core/ext/filters/message_size/message_size_filter.cc, line 351 at r1 (raw file):

  new (chand) channel_data();
  chand->limits = get_message_size_limits(args->channel_args);
  // Get method config table from channel args.

As mentioned above, this is no longer needed.

src/core/ext/filters/message_size/message_size_filter.cc, line 401 at r1 (raw file):

// Used for GRPC_CLIENT_DIRECT_CHANNEL and GRPC_SERVER_CHANNEL. Adds the filter
// only if message size limits or service config is specified.

I don't think this is going to work, since we're no longer passing the service config via channel args. I think we'll need to just add this filter unconditionally, since we won't know whether or not it's going to be used at channel stack initialization time.

src/core/ext/filters/message_size/message_size_parser.h, line 26 at r2 (raw file):

namespace grpc_core {

class MessageSizeParsedObject : public ServiceConfigParsedObject {

These classes can go in the message_size_filter.cc file. They should not be needed outside of this filter.

src/core/lib/channel/context.h, line 38 at r1 (raw file):

  GRPC_CONTEXT_TRAFFIC,

  GRPC_SERVICE_CONFIG,

See my comment elsewhere about combining these two indexes into one.

Also, please add a comment describing what it's for and what type the value will be.

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

class Optional {
 public:
  Optional() = default;

Do these changes match the API provided by absl::Optional? We should stick with what that API provides, to make it easier to switch to that implementation later.

src/core/lib/iomgr/error.h, line 33 at r1 (raw file):

#include "src/core/lib/debug/trace.h"
#include "src/core/lib/gprpp/inlined_vector.h"

This doesn't seem to be used.

test/core/client_channel/service_config_test.cc, line 380 at r2 (raw file):

class ClientChannelParserTest : public ::testing::Test {
 protected:
  void SetUp() override {

Why is this needed? Can't we just get the parsed object index using ClientChannelServiceConfigParser::client_channel_service_config_parser_index()?

test/core/client_channel/service_config_test.cc, line 454 at r2 (raw file):

  gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
  ASSERT_TRUE(error != GRPC_ERROR_NONE);
  std::regex e(

This error-checking code is fairly repetitive -- each test has the same several lines of code. Would it make sense to refactor this into a helper function that takes a grpc_error* and regexp string and returns true if it matches?

test/core/client_channel/service_config_test.cc, line 466 at r2 (raw file):

}

TEST_F(ClientChannelParserTest, InvalidgRPCLbLoadBalancingConfig) {

s/gRPCLb/Grpclb/

test/core/client_channel/service_config_test.cc, line 513 at r2 (raw file):

TEST_F(ClientChannelParserTest, ValidLoadBalancingPolicy) {
  const char* test_json = "{\"loadBalancingPolicy\":\"pick_first\"}";

Let's also test the case where the value is in all-caps.

test/core/client_channel/service_config_test.cc, line 820 at r2 (raw file):

      ".*)(Client channel "
      "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
      "*)(field:maxAttempts error:should be atleast 2)"));

Nit: "at least" should be two separate words.

test/core/client_channel/service_config_test.cc, line 903 at r2 (raw file):

      "      \"maxAttempts\": 1,\n"
      "      \"initialBackoff\": \"1s\",\n"
      "      \"maxBackoff\": \"120sec\",\n"

s/sec/s/

test/core/client_channel/service_config_test.cc, line 936 at r2 (raw file):

      "      \"maxAttempts\": 1,\n"
      "      \"initialBackoff\": \"1s\",\n"
      "      \"maxBackoff\": \"120sec\",\n"

s/sec/s/

test/core/client_channel/service_config_test.cc, line 961 at r2 (raw file):

class MessageSizeParserTest : public ::testing::Test {
 protected:
  void SetUp() override {

Similar question to the above. Can't we just access the via the normal mechanism, so we don't need to re-register the parser every time?

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

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

Done.

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

Done.

src/core/ext/filters/client_channel/client_channel_plugin.cc, line 56 at r1 (raw file):

Done.

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

Done.

src/core/ext/filters/client_channel/lb_policy_factory.h, line 30 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy_factory.h, line 34 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 50 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 62 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 67 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 124 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 134 at r1 (raw file):

By doing this over here, we avoid having to get the global parsed object again in ProcessLbPolicyName and performing a few nullptr checks

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 147 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 415 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 378 at r2 (raw file):

Done

src/core/ext/filters/client_channel/service_config.h, line 108 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/service_config.h, line 117 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/service_config.h, line 127 at r1 (raw file):

Done.

src/core/ext/filters/client_channel/service_config.h, line 179 at r1 (raw file):

Done.

src/core/ext/filters/message_size/message_size_filter.cc, line 149 at r1 (raw file):

It is needed for Direct Channels

src/core/ext/filters/message_size/message_size_filter.cc, line 351 at r1 (raw file):

needed for direct channels

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

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

Done.

src/core/ext/filters/client_channel/client_channel_factory.h, line 39 at r1 (raw file):

Done.

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

Done.

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

Done.

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 44 at r1 (raw file):

If this is a json error, it should get caught by the json parser.

The changes here are definitely moving in the right direction! I assume that you're still working on the remaining open comments.

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

Reviewed 24 of 27 files at r3.
Reviewable status: all files reviewed, 44 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @yashykt)

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

    int num_args_to_add = 0;
    if (chand_->service_config_ != nullptr) {
      /*const auto* health_check_object = static_cast<HealthCheckParsedObject*>(

Why is this commented out? It looks like it should work.

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

         strcmp(service_config_json, chand->info_service_config_json_.get()) !=
             0);
    chand->info_service_config_json_.reset(gpr_strdup(service_config_json));

Nit: Might be a good idea to do the gpr_strdup() before acquiring chand->info_mu_, so that we only need to do a std::move() while holding the lock.

src/core/ext/filters/client_channel/client_channel_factory.h, line 26 at r3 (raw file):

#include <grpc/impl/codegen/grpc_types.h>

#include "src/core/ext/filters/client_channel/health/health_check_parser.h"

This is no longer needed.

src/core/ext/filters/client_channel/client_channel_plugin.cc, line 38 at r3 (raw file):

#include "src/core/ext/filters/client_channel/resolver_result_parsing.h"
#include "src/core/ext/filters/client_channel/retry_throttle.h"
#include "src/core/ext/filters/message_size/message_size_parser.h"

This is no longer needed.

src/core/ext/filters/client_channel/lb_policy_registry.h, line 62 at r3 (raw file):

  /// Validates if the deprecated loadBalancingPolicy field can be parsed.
  /// Returns GRPC_ERROR_NONE if successfully parsed.
  static grpc_error* ParseDeprecatedLoadBalancingPolicy(const grpc_json* json);

Having this method here does make sense, but I fear that Sanjay will object, since he wants to be able to use the LB policy API for things other than gRPC, and this deprecated field is really gRPC-specific. So let's move this back to the client_channel parsing code.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 43 at r1 (raw file):

That's the type used in the retry throttling code, so it seems better to use the same types here. That way, we can support any arbitrarily large value that anyone might want to configure.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 134 at r1 (raw file):

How about passing in the parsed object from the caller? Could do the same thing for ProcessServiceConfig().

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 95 at r3 (raw file):

    return;
  }
  const grpc_arg* channel_arg =

Could move all of these lines inside of the if block below, since there's no point in getting the channel arg or parsing the URI if we have no retry throttling params.

src/core/ext/filters/client_channel/subchannel.h, line 26 at r3 (raw file):

#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
#include "src/core/ext/filters/client_channel/connector.h"
#include "src/core/ext/filters/client_channel/health/health_check_parser.h"

This is no longer needed.

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 33 at r1 (raw file):

Does that mean it can't happen? If so, maybe make it an assertion?

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 44 at r1 (raw file):

Maybe make it an assertion?

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

      // field.
      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
          "field:loadBalancingPolicy error:Xds Parser has required field - "

This could also happen if the xds policy is selected via the client API, so the error message should probably mention that case as well.

src/core/ext/filters/message_size/message_size_filter.cc, line 125 at r1 (raw file):

That's a good point. But I don't think it makes sense for each individual filter that needs service config data to have to duplicate this functionality.

An alternative would be to add a service config filter that sits at the top of the filter stack for direct channels. Its purpose would be to get the service config from the channel arg, parse it, and populate the call context for each call. That way, any filter lower in the stack that needs the service config can access it the same way, regardless of whether the channel is a direct channel or a normal client channel.

src/core/ext/filters/message_size/message_size_filter.cc, line 401 at r1 (raw file):

Good point!

test/cpp/microbenchmarks/bm_call_create.cc, line 33 at r3 (raw file):

#include "src/core/ext/filters/client_channel/client_channel.h"
#include "src/core/ext/filters/client_channel/client_channel_factory.h"

This is no longer needed.

Reviewed 2 of 27 files at r3, 41 of 44 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @yashykt)

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

  grpc_connectivity_state_tracker state_tracker_;
  ExternalConnectivityWatcher::WatcherList external_connectivity_watcher_list_;
  const char* health_check_service_name_ = nullptr;

If this is not owned by the ChannelData object, how do we know that it will remain alive until we're no longer using it? It looks like this is pointing into the parsed service config data, but we're not holding a ref to the service config here in the control plane combiner; that gets held in the data plane combiner. So this could lead to weird bugs where the control plane starts using the old value while the data plane swaps out the service config, thus causing a crash.

I think we should make a copy here, and this data member should be a UniquePtr<char>.

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

 public:
  ServiceConfigSetter(
      ChannelData* chand, UniquePtr<char> server_name,

Instead of copying and passing in the server name each time the service config is updated, I suggest having ChannelData store the server name at construction time and provide an accessor method to be used by ServiceConfigSetter. The server name will remain the same for the lifetime of the client channel, so there should be no lifetime issues.

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

  *lb_policy_name = chand->info_lb_policy_name_.get();
  *lb_policy_config = resolver_result.lb_policy_config();
  chand->health_check_service_name_ =

Please move this up to line 1138, so that it's not in the "Return results" section of the function.

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

      ServiceConfig::CallData(chand->service_config(), path_);
  if (!service_config_call_data_.empty()) {
    call_context_[GRPC_SERVICE_CONFIG_CALL_DATA].value =

I like the way you're storing this directly in the filter's call data here -- this is a nice way to avoid an allocation!

src/core/ext/filters/client_channel/lb_policy_registry.h, line 62 at r3 (raw file):

From the perspective of this API, this method doesn't really make sense. Someone looking at this is going to wonder what the difference is between the existing LoadBalancingPolicyExists() method and this new one, and it's not at all obvious.

Instead, how about just adding a bool* requires_config argument to the existing LoadBalancingPolicyExists() method, so that it can return whether or not a config is required? That approach has the added benefit that we can check whether the policy specified by the legacy field actually exists, which I don't think we're checking today.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 43 at r1 (raw file):

I agree that this seems like a bit of an odd choice. I think it used to use a different type and was changed to use this type at some point, but I don't remember the reason.

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 116 at r1 (raw file):

The main reason to remove it is that its API is really ugly, and this PR is providing a much cleaner way to do the same thing, so it seems reasonable to clean up the ugliness that is no longer necessary. The logic that remains here is IMHO better provided via some simple helper functions in the client_channel ChannelData object.

If you'd prefer not to do that in this PR, I can do that in a separate PR. It's up to you.

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

        retry_throttling_(retry_throttling) {}

  Optional<RetryThrottling> retry_throttling() const {

Nit: Let's keep the accessor methods in the same order as the data members and the ctor parameters.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 107 at r5 (raw file):

    grpc_uri* uri = grpc_uri_parse(server_uri, true);
    GPR_ASSERT(uri->path[0] != '\0');
    server_name_ = uri->path[0] == '/' ? uri->path + 1 : uri->path;

Looks like the server name is not actually needed here anymore, so all of the code for reading the channel arg and parsing it as a URI can go away.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 128 at r5 (raw file):

        char* lb_policy_name = lb_policy_name_.get();
        for (size_t i = 0; i < strlen(lb_policy_name); ++i) {
          lb_policy_name[i] = tolower(lb_policy_name[i]);

We're now doing this lower-case conversion in ClientChannelServiceConfigParser::ParseGlobalParams(), so we don't need to do it here anymore.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 355 at r5 (raw file):

        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "field:loadBalancingConfig error:Duplicate entry"));
      } else {

Even if the entry is a duplicate, shouldn't we still parse the duplicate and report any errors we find?

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 370 at r5 (raw file):

        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "field:loadBalancingPolicy error:Duplicate entry"));
        continue;

Can't this error and the next one both happen in the same pass of the loop? If so, we should report them both.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 400 at r5 (raw file):

        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "field:retryThrottling error:Type should be object"));
      } else if (retry_throttling.has_value()) {

Same here.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 414 at r5 (raw file):

                  "field:retryThrottling field:maxTokens error:Duplicate "
                  "entry"));
            } else if (sub_field->type != GRPC_JSON_NUMBER) {

Same here.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 432 at r5 (raw file):

                  "field:retryThrottling field:tokenRatio error:Duplicate "
                  "entry"));
            } else if (sub_field->type != GRPC_JSON_NUMBER) {

Same here.

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

  void* process_resolver_result_user_data_ = nullptr;
  UniquePtr<char> child_policy_name_;
  RefCountedPtr<ParsedLoadBalancingConfig> child_lb_config_ = nullptr;

No need for the = nullptr, since that's the default.

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

  // Process the resolver result.
  const char* lb_policy_name = nullptr;
  RefCountedPtr<ParsedLoadBalancingConfig> lb_policy_config = nullptr;

No need for = nullptr.

src/core/ext/filters/client_channel/service_config.h, line 155 at r2 (raw file):

If you want to move this to error.h, that's fine. But if it stays here, then it's intended for use in the service config code only, in which case it can be tailored to that use-case.

src/core/ext/filters/client_channel/service_config.h, line 156 at r2 (raw file):

If you want to move it to error.h, I'd be fine with that. But I don't think it should be part of the ServiceConfig class's public API, since it's not really something that should be provided by this class.

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

    virtual ~Parser() = default;

    virtual UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(

I don't think the ServiceConfig:: prefix is needed here, since we're already in that scope.

Same thing throughout this file -- I suspect this prefix isn't needed anywhere within the class.

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

      ServiceConfigObjectsVector;

  class CallData {

Please add a comment documenting what this is for and how it is used.

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

      ServiceConfigObjectsVector;

  class CallData {

It might be useful to have a method on this class for accessing the parsed object at a given index, for both global and per-method data.

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

  class CallData {
   public:
    CallData() = default;

Why does this need a zero-argument ctor?

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

    }

    RefCountedPtr<ServiceConfig> service_config() { return service_config_; }

This should probably return the raw pointer, not the RefCountedPtr<>. I suspect most callers of this won't actually need their own ref, so we shouldn't force them to increment the ref-count. (Note that any caller that does actually need a ref can get it by calling the object's Ref() method.)

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

    }

    bool empty() const { return service_config_ == nullptr; }

Why is this needed? Can't a caller determine this by checking the result of the service_config() accessor method?

src/core/ext/filters/client_channel/service_config.cc, line 316 at r5 (raw file):

}

size_t ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser> parser) {

I suspect that the ServiceConfig:: prefix is not needed here.

src/core/ext/filters/client_channel/health/health_check_parser.h, line 39 at r1 (raw file):

I don't think we're ever going to want to do that. The health check mechanism is for use in subchannels, and subchannels are an inherent part of the client_channel functionality.

Once I merge #18839 and the follow-up PR that I'm planning to write, the health-checking configuration will actually be completely invisible to the LB policies and the subchannel_list code, so the only place it will ever be used is in the client_channel filter itself.

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 56 at r1 (raw file):

Can't both errors happen in the same pass of the loop (i.e., a duplicate entry that is of a type other than string)?

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

class ParsedGrpcLbConfig : public ParsedLoadBalancingConfig {
 public:
  ParsedGrpcLbConfig(RefCountedPtr<ParsedLoadBalancingConfig> child_policy)

This needs to be explicit.

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

    child_policy_config_ = nullptr;
  }

Nit: Please remove blank lines within functions.

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

      void UpdateLocked(
          xds_grpclb_serverlist* serverlist,
          const RefCountedPtr<ParsedLoadBalancingConfig>& child_policy_config,

Suggest just passing in a raw pointer here, rather than a const reference to a RefCountedPtr<>.

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

    void UpdateLocked(
        const LocalityList& locality_list,
        const RefCountedPtr<ParsedLoadBalancingConfig>& child_policy_config,

Same here.

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

    InlinedVector<grpc_error*, 3> error_list;
    const char* balancer_name = nullptr;
    RefCountedPtr<ParsedLoadBalancingConfig> child_policy = nullptr;

The = nullptr is not needed on these two lines.

src/core/ext/filters/message_size/message_size_filter.cc, line 125 at r1 (raw file):

Okay, I'll buy that argument. But please add a TODO here (assigned to me, if you'd like) that if we ever get a second filter in the subchannel stack that needs the service config, we should refactor this code to avoid duplicating it.

src/core/ext/filters/message_size/message_size_filter.cc, line 41 at r5 (raw file):

static void recv_trailing_metadata_ready(void* user_data, grpc_error* error);

namespace {

This can go inside of the grpc_core namespace block below, since it's used only by the code there.

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

Same question: are we sure that this change keeps the interface consistent with the absl version?

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

class Optional {
 public:
  Optional() { value_ = {}; }

I think you can say value_ = T().

Also, you can probably initialize this where the data member is declared rather than defining a zero-argument ctor.

test/core/client_channel/service_config_test.cc, line 380 at r2 (raw file):

Can we just register the test suite's custom parsers once at startup and reuse them for all tests?

If there's some reason that won't work, this is fine. But if there's a way we can simplify, we should.

test/core/client_channel/service_config_test.cc, line 903 at r2 (raw file):

Why? The regexp below doesn't seem to be checking for this error, and there's a separate test case for that above.

test/core/client_channel/service_config_test.cc, line 936 at r2 (raw file):

Same question as above.

(Sorry, I accidentally hit "publish" before writing the following.)

This looks really good! Most of the remaining comments are minor.

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

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

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

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

You're right. Done!

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

Good idea, done!

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

done!

src/core/ext/filters/client_channel/lb_policy_registry.h, line 62 at r3 (raw file):

I like that idea! Done

src/core/ext/filters/client_channel/resolver_result_parsing.h, line 116 at r1 (raw file):

Cool, I'll do it in a separate PR. I would like to avoid making too many new changes in this PR

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 107 at r5 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 128 at r5 (raw file):

Good catch. Done!

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 355 at r5 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 370 at r5 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 400 at r5 (raw file):

done for all duplicate entries

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 414 at r5 (raw file):

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 432 at r5 (raw file):

Done.

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

Done.

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

Done.

src/core/ext/filters/client_channel/service_config.h, line 155 at r2 (raw file):

moved to error.h

src/core/ext/filters/client_channel/service_config.h, line 156 at r2 (raw file):

moved to error.h

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

Done.

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

Done.

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

Done.

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

so that it can be default constructed in client_channel's call_data

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

Done.

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

You're right, removed

src/core/ext/filters/client_channel/service_config.cc, line 316 at r5 (raw file):

interesting! done!

src/core/ext/filters/client_channel/health/health_check_parser.h, line 39 at r1 (raw file):

combined with client_channel parser

src/core/ext/filters/client_channel/health/health_check_parser.cc, line 56 at r1 (raw file):

agreed, continuing parsing in all locations for duplicate entries

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

Done.

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

Done.

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

Done.

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

Done.

src/core/ext/filters/message_size/message_size_filter.cc, line 125 at r1 (raw file):

Done.

src/core/ext/filters/message_size/message_size_filter.cc, line 41 at r5 (raw file):

Done.

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

Yes, this does not change the interface. For absl::Optional, we actually get a bad access exception if try to access the value when it is empty. This change just allows us to go around the compiler warnings.

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

I tried doing it where the data member was declared but I got a weird clang error that seems to be a bug with clang itself

test/core/client_channel/service_config_test.cc, line 380 at r2 (raw file):

The custom parsers can not be resused since some parsers just always add errors. We would need to unregister them.

test/core/client_channel/service_config_test.cc, line 903 at r2 (raw file):

you're right :) The mistake was actually a mistake and not intentional here :D

test/core/client_channel/service_config_test.cc, line 936 at r2 (raw file):

Done

This is looking excellent! Remaining comments are all fairly minor.

Reviewed 18 of 33 files at r6.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn, @AspirinSJL, @markdroth, and @yashykt)

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

    return;
  }
  grpc_uri* uri = grpc_uri_parse(server_uri, true);

Need to call grpc_uri_destroy() on this when we're done with it.

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

  UniquePtr<char> parsed_deprecated_lb_policy_;
  Optional<RetryThrottling> retry_throttling_;
  const char* health_check_service_name_ = nullptr;

No need for the = nullptr in this particular case, since this field is always set by the ctor.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 414 at r5 (raw file):

Looks like you actually missed this one.

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

  health_check_service_name_ = parsed_object->health_check_service_name();
  service_config_json_ = service_config_->service_config_json();
  if (!parsed_object) {

Can probably reverse this and just say:

if (parsed_object != nullptr) {
  retry_throttle_data_ = parsed_object->retry_throttling();
}

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

    return;
  }
  if (parsed_object->retry_throttling().has_value()) {

I think this conditional is not needed anymore; we can copy the Optional<> value regardless of whether or not it's set.

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

grpc_channel_args* GrpcLb::CreateChildPolicyArgsLocked(
    bool is_backend_from_grpclb_load_balancer) {
  grpc_arg args_to_add[2] = {

Why change away from using InlinedVector<> here? It seems easier to do that than to mess with C arrays and manually tracking the size of the array.

src/core/ext/filters/message_size/message_size_filter.cc, line 74 at r6 (raw file):

        error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
            "field:maxResponseMessageBytes error:Duplicate entry"));
      }  // Duplicate, conitnue parsing

s/conitnue/continue/

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

Hmm... actually, stepping back for a moment, why are we getting the "may be used uninitialized" error to begin with? Doesn't that imply that the code is actually trying to access the member when it's not set, in which case it's a bug in the calling code and not a problem here?

It also occurs to me that if type T has a zero-arg ctor, then it should be initialized that way just from being declared as a data member in the first place, so reassigning it won't actually help. So I'm now curious as to why this change is actually making any difference in the first place.

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

That seems unlikely. Compiler bugs do happen, but they're rare -- I generally find that when I think something may be a compiler bug, it's actually just that I didn't understand what the compiler was telling me. What's the actual error you're seeing?

src/core/lib/iomgr/error.h, line 200 at r6 (raw file):

// them. If the vector is empty, return GRPC_ERROR_NONE.
template <size_t N>
static grpc_error* GRPC_ERROR_CREATE_FROM_VECTOR(

All-caps names are for macros, not functions.

The other GRPC_ERROR_CREATE_* macros get the file and line number of the caller to encode in the error. This should probably do the same.

So, I think the right thing here is to rename this function to grpc_error_create_from_vector() and give it parameters for the file and line numbers, and then add a GRPC_ERROR_CREATE_FROM_VECTOR() macro that provides the current API and automatically passes the file and line number params to this function.

test/core/client_channel/service_config_test.cc, line 380 at r2 (raw file):

Good point. Okay, this is fine.

Reviewable status: 17 of 24 files reviewed, 11 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @markdroth)

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

Done.

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

Done.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 414 at r5 (raw file):

Sorry about that

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

Done.

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

Done.

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

looks like an effect of a bad merge. I'll fix it. Thanks! :)

src/core/ext/filters/message_size/message_size_filter.cc, line 74 at r6 (raw file):

well spotted

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

we are copying the Optional objects at some places. The copy constructor also ends up copying the value which might be uninitialized. For primitive data-types, like int, bool, they end up not being initialized. By zero-initializing, we get a default value and copying doesn't complain anymore.

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

defaulted default constructor of '' cannot be used by non-static data member initializer which appears before end of class definition.. It is very likely that I don't understand what it's actually trying to tell me. I went ahead with the easiest fix that worked :)

src/core/lib/iomgr/error.h, line 200 at r6 (raw file):

good idea!

Just a couple of minor nits remaining!

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

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

Looks like you made this change in the wrong place -- you changed it in ProcessedResolverResult instead of in ClientChannelGlobalParsedObject.

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 414 at r5 (raw file):

No worries. I mean, it's not like this PR is large or invasive or anything... :)

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

Maybe a better solution would be to provide a copy ctor and copy-assignment operator that copy the value only if it's set? It seems like that would actually adhere more closely to the behavior of absl::optional. It may make a difference if the ctor of type T has side-effects.

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

A quick search yields the following:

https://stackoverflow.com/questions/17430377/error-when-using-in-class-initialization-of-non-static-data-member-and-nested-cl

I don't fully understand this, but it appears to be a known issue in the C++ spec.

This is exactly why I never assume that something is a compiler bug. Experience has taught me that compiler developers are a lot smarter than I am. :)

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

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

The code author himself gets confused on the right parameter to change :D

src/core/lib/gprpp/optional.h, line 29 at r1 (raw file):

I have tried that. With code something like
if(set_) {
this->value_ = other->value_;
}

The compiler does not seem to realize that value is only copied when set_ is true.

I am going to avoiding changes to Optional in this PR.

src/core/lib/gprpp/optional.h, line 29 at r5 (raw file):

wondering if the compiler could do something smarter here.

Just one issue left. I don't understand why the Optional<> initializations are helping here.

Reviewed 3 of 3 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @yashykt)

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 419 at r8 (raw file):

        continue;
      }
      Optional<int> max_milli_tokens = Optional<int>();

I really don't see why this makes any difference. The value we're assigning here is exactly the same default value it should get if there was no assignment, because both are using the zero-arg ctor.

What is the exact compiler error that occurs if we don't do this?

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

  GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
  InlinedVector<grpc_error*, 4> error_list;
  Optional<bool> wait_for_ready = Optional<bool>();

Same question here.

src/core/lib/gprpp/optional.h, line 42 at r8 (raw file):

 private:
  bool set_ = false;
  T value_;

Might as well move this back up to the previous line, so that there are no longer any changes in this file in this PR.

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

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 419 at r8 (raw file):

This is something thing I learnt an hour back. Explanation-
struct C {
int x;
}

C c;
// c.x is garbage

C c = C(); // uses value initialization
assert(c.x == 0);

reference - https://stackoverflow.com/questions/2417065/does-the-default-constructor-initialize-built-in-types

src/core/lib/gprpp/optional.h, line 42 at r8 (raw file):

Done.

Reviewed 2 of 2 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @yashykt)

src/core/ext/filters/client_channel/resolver_result_parsing.cc, line 419 at r8 (raw file):

I see... absl::optional<> must do something clever to avoid this.

I guess for now let's go back to initializing the value in the Optional<> zero-arg ctor. This seems wrong, but it's probably actually not any more wrong than our existing Optional<> implementation, because if the value type does have a zero-arg ctor with side-effects, the existing implementation will already be calling it, even without explicit initialization. But please add a TODO there about fixing it to not actually initialize the value.

This looks fantastic! Thanks for doing this!

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