Refactor code for gracefully switching child LB policies. by markdroth · Pull Request #22101 · grpc/grpc

markdroth · 2020-02-21T19:47:54Z

@muxi, this will probably be useful to you in the RLS policy you're implementing.

markdroth · 2020-02-21T20:36:28Z

Sorry, had to do a force-push to bring in some changes. The last commit is still what's new here.

markdroth · 2020-02-21T23:45:48Z

The main changes to look at are now in commits 07d42d8 and fb9cbcb.

markdroth · 2020-02-26T19:13:31Z

I've merged in the changes that this depended on, so this PR now contains only its own changes. Feel free to review. Thanks!

muxi · 2020-02-27T20:49:59Z

src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc

@@ -0,0 +1,277 @@
+//
+// Copyright 2018 gRPC authors.


This is basically moving code from another file, and the code was originally written in 2018, so I'm going to keep that date.

muxi · 2020-02-27T20:50:06Z

src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h

@@ -0,0 +1,66 @@
+//
+// Copyright 2018 gRPC authors.


The code was originally written in 2018.

muxi · 2020-02-27T21:16:44Z

src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc

+    }
+    auto& lb_policy =
+        child_policy_ == nullptr ? child_policy_ : pending_child_policy_;
+    lb_policy = CreateChildPolicy(child_policy_name, *args.args);


Why would we want to dereference the pointer here and then take address again in this function? Can we pass const grpc_channel_args* instead?

The handing of grpc_channel_args is inconsistent all across our codebase, unfortunately. It's one of our oldest types, long predating the conversion from C to C++, so most of our older interfaces take it as a pointer. However, the C++ convention is to pass read-only arguments as const references, so that's what we do in newer code.

At some point, I would like to find time to convert grpc_channel_args to C++ and fix all of our code to use a more idiomatic interface. But until then, these inconsistencies will have to remain.

Sounds reasonable. Is this convention a gRPC thing or it's in the cpp style? I searched through cppstype a bit but did not find any reference

It is in the style guide, but confusingly, it's buried in the middle of a section about output parameters, even though it's talking about input parameters:

https://google.github.io/styleguide/cppguide.html#Output_Parameters

"Input parameters are usually values or const references"

Thanks for the pointer :)

muxi · 2020-02-27T21:30:59Z

src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc

+          parent_->child_policy_->interested_parties(),
+          parent_->interested_parties());
+      parent_->child_policy_ = std::move(parent_->pending_child_policy_);
+    } else if (!CalledByCurrentChild()) {


This is very nit, but would it be a little bit less awkward if you change the negative condition to a positive condition? When I read this line I was like "why would we even care if it's not a pending and not a current child", then after 3 seconds it became "oh yea we do not care about this case indeed". Changing to a positive condition would make it flow easier I guess.

Can you give an example of what you mean by "a positive condition"? I'm not sure what you mean.

Note that I can't just change this to check that it is from the current child and then move line 67 inside of the body here, because we want line 67 to run even if the case of a pending child that has just gone READY and been swapped into being the current child.

Note that I can't just change this to check that it is from the current child

That was what I meant. And I see the point here; you are right.

muxi · 2020-02-27T21:33:39Z

src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc

+        parent_->pending_child_policy_ != nullptr
+            ? parent_->pending_child_policy_.get()
+            : parent_->child_policy_.get();
+    if (child_ != latest_child_policy) return;


Could you explain why it's only the latest child policy that can trigger a reresolution? Maybe with a line of comment.

muxi · 2020-02-27T21:46:32Z

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

  if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_glb_trace)) {
-    gpr_log(GPR_INFO, "[grpclb %p] Created new child policy %s (%p)", this,
-            name, lb_policy.get());
+    gpr_log(GPR_INFO, "[grpclb %p] Created new child policy (%p)", this,


Since we know it's a child policy handler that got created, maybe we can log "Created new child policy handler" to tell things apart better. But current state is also fine too. Ditto to the gpr_log a few lines above.

muxi · 2020-02-27T21:50:59Z

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

+          {"round_robin", Json::Object()},
+      }};
+    } else {
+      child_policy_config_json = it->second;


This could be a big copy. Consider use pointer for child_policy_config_json?

muxi · 2020-02-27T22:01:15Z

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

+          {"round_robin", Json::Object()},
+      }};
+    } else {
+      child_policy_json = it->second;


Ditto. Can we use pointer instead to avoid possible copy?

muxi · 2020-02-27T22:06:25Z

src/core/ext/filters/client_channel/resolving_lb_policy.h

  OrphanablePtr<LoadBalancingPolicy> CreateLbPolicyLocked(
-      const char* lb_policy_name, const grpc_channel_args& args,
-      TraceStringVector* trace_strings);
+      const grpc_channel_args& args);


Ditto. Use pointer instead of reference here?

See my reply elsewhere. This is the normal C++ convention for passing a read-only parameter.

markdroth · 2020-02-28T16:17:09Z

Known issues: #22181 #18504 #20198 #21993 #21364 #20677

Refactor code for gracefully switching child LB policies.

markdroth added the release notes: no Indicates if PR should not be in release notes label Feb 21, 2020

markdroth requested a review from muxi February 21, 2020 19:47

markdroth force-pushed the lb_graceful_switcher branch from f105e86 to 07d42d8 Compare February 21, 2020 20:35

markdroth mentioned this pull request Feb 26, 2020

Always construct LB policy config, even when only the policy name is specified. #22100

Merged

markdroth force-pushed the lb_graceful_switcher branch from c0e4a3c to 7ca20bd Compare February 26, 2020 19:12

muxi reviewed Feb 27, 2020

View reviewed changes

muxi approved these changes Feb 28, 2020

View reviewed changes

Refactor code for gracefully switching child LB policies.

19e28b1

markdroth force-pushed the lb_graceful_switcher branch from a64b259 to 19e28b1 Compare February 28, 2020 16:18

markdroth merged commit 5e98ad4 into grpc:master Feb 28, 2020

markdroth deleted the lb_graceful_switcher branch February 28, 2020 20:34

gnossen pushed a commit to gnossen/grpc that referenced this pull request Mar 13, 2020

Merge pull request grpc#22101 from markdroth/lb_graceful_switcher

2957004

Refactor code for gracefully switching child LB policies.

This was referenced Mar 13, 2020

Backport #22101 to 1.28 (Refactor code for gracefully switching child LB policies.) #22335

Closed

Backport xDS Bug Fixes and Test Improvements #22337

Merged

Conversation

markdroth commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdroth commented Feb 21, 2020

Uh oh!

markdroth commented Feb 21, 2020

Uh oh!

markdroth commented Feb 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

markdroth commented Feb 21, 2020 •

edited

Loading