Simplify LB policy refcounting.#13857
Merged
markdroth merged 1 commit intogrpc:masterfrom Jan 9, 2018
Merged
Conversation
|
|
dgquintas
reviewed
Dec 21, 2017
| const grpc_lb_policy_pick_args inputs = { | ||
| calld->pick.initial_metadata = | ||
| calld->initial_metadata_batch->payload->send_initial_metadata | ||
| .send_initial_metadata, |
Contributor
There was a problem hiding this comment.
the trailing comma should be a semicolon.
Member
Author
There was a problem hiding this comment.
Doh! Thanks for catching this. Fixed.
|
|
|
|
dgquintas
approved these changes
Dec 21, 2017
Contributor
dgquintas
left a comment
dgquintas
left a comment
There was a problem hiding this comment.
This PR looks great, really makes things simpler. Thanks Mark!
| typedef struct glb_lb_policy glb_lb_policy; | ||
|
|
||
| /// Linked list of pending pick requests. It stores all information needed to | ||
| //// eventually call (Round Robin's) pick() on them. They mainly stay pending |
Contributor
There was a problem hiding this comment.
nit: three slashes should be enough :)
|
|
|
|
|
|
Merged
|
|
|
AspirinSJL
approved these changes
Jan 9, 2018
Contributor
AspirinSJL
left a comment
AspirinSJL
left a comment
There was a problem hiding this comment.
I roughly read the changes and understood the main idea.
|
|
|
markdroth
force-pushed
the
lb_policy_ref_simplification
branch
from
6eea1dc to
c0febd3
Compare
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR eliminates the two types of refs for LB policies (strong vs. weak) and replaces them with a single type of reference.
To make this work, we ensure that there is only one external owner of the LB policy, which is responsible for explicitly calling
glb_lb_policy_shutdown_locked(). Prior to this PR, there were two external owners of an LB policy, both in the client_channel filter: the filter'schannel_datastruct was the primary owner, but we were also holding a reference in the filter'scall_dataso that when we switch to a new LB policy, we don't shut down the old policy until it has finished the picks that were already in flight with that old LB policy. This PR eliminates the reference held by thecall_datastruct and provides a different solution for switching LB policies: now, when the old LB policy is shut down, if there is a new LB policy created, it hands off any pending picks to the new LB policy.In order to make it easier for one LB policy to hand off picks to another, I moved all parameters used in a pick to a single struct, called
grpc_lb_policy_pick_state. This wound up having a very nice side effect, which is that this struct can double as thepending_pickstruct inside the PF and RR policies, thus eliminating some unnecessary allocations. The logic in the grpclb policy was more complex, so a wrapper struct was still needed there, but even in that case, the logic is much simpler now (e.g., I was able to combine thepending_pickandwrapped_rr_closurestructs), and I've eliminated some unnecessary allocations there too.While I was working on the grpclb code, I also changed it to not hold a ref the RR policy for each pick. Since we no longer have any cases where the RR policy will be shut down out from under us, these references are no longer necessary. I was also able to greatly simplify the
pending_pingsstruct.In a subsequent PR, I will convert the LB policy code to use a C++ API similar to the one I wrote for the resolver (see #13684).