A119: Slicer LB Policy - #551
Conversation
|
@markdroth @ejona86 @dfawley |
|
I'm going to be making some structural changes to the spec based on our review today. Will ping here once it is ready for review. @markdroth @ejona86 @dfawley @shivaspeaks @pawbhard Please expect some changes to the spec, but not wholesale changes. So, you could start looking at it from an implementation pov. Thanks. |
|
The PR is ready to be looked at. |
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks really good!
I have a lot of comments here, but they're mostly fine details or clarifications -- the high-level shape of this is solid, and the doc does a good job of describing it.
Please let me know if you have any questions. Thanks!
| @@ -0,0 +1,931 @@ | |||
| # A119: Slicer LB Policy | |||
There was a problem hiding this comment.
Nit: Please follow the gRFC template's formatting exactly:
https://github.com/grpc/proposal/blob/master/GRFC-TEMPLATE.md
As an example, the formatting you're using here causes the title to appear in the outline, which is undesirable.
|
|
||
| * Application servers | ||
| * Kubernetes pods within a cluster | ||
| * Regions within a multi-regional service deployment |
There was a problem hiding this comment.
Since we've decided not to support slicer as a locality-picking policy that can delegate to a child policy for endpoint picking, we're not going to support this use-case, so it might be better to remove this bullet.
| The LB policy must support a fallback mechanism that utilizes all endpoints | ||
| provided by the Name Resolver. There are two types of fallback: | ||
|
|
||
| * Per-RPC fallback: |
There was a problem hiding this comment.
This might be better described as "per-slice fallback".
| * This happens when the following conditions are met: | ||
| * No valid assignments have been received from the sharding service | ||
| * Endpoints received from the Name Resolver | ||
| * Fallback timer has expired |
There was a problem hiding this comment.
I don't see anything in this document that actually describes what this timer is or how it works. I think we need to flesh that out here.
There was a problem hiding this comment.
I added a new sub-section here to talk about it.
I remember you didn't want us to start the timer everytime we create a new channel, but it feels wrong to me to use assignments from a server that we are no longer supposed to talk to (when the target URI changes). But since we are using a channel_factory_key and not just the target_uri, the LB policy cannot distinguish if it is talking to the same sharding server as before or a new one. All it knows is that the key has changed and that it has created a new channel.
Also, I believe that changes to this key will be a rare event. So, I'm in favor of restarting the timer whenever the policy creates a new channel. I'd like to hear your thoughts.
|
|
||
| The LB policy receives the following information from the Name Resolver apart from its configuration: | ||
|
|
||
| * Endpoints with an optional hostname attribute |
There was a problem hiding this comment.
We need to explain that if the hostname attribute is missing, we will use the address as the name.
| // to receive sharding assignments from and to send load reports to. | ||
| config.core.v3.GrpcService grpc_service = 1 | ||
|
|
||
| // An identifier sent to the sharding service. Assignments and load reports are |
There was a problem hiding this comment.
Please remove reference to load reporting, since that's not part of this design.
| While connecting to backends eagerly like `pick_first` or `round_robin` would | ||
| work for the first case, it would be extermely wasteful in the second case. Most | ||
| of our known use-cases fall into the second bucket and optimizing for that seems | ||
| prudent. |
There was a problem hiding this comment.
It's probably also worth noting that supporting lazy connection establishment will work fine for both use-cases, because in the reverse-proxy case, it will quickly wind up establishing connections to all endpoints anyway.
|
|
||
| * [A42: xDS Ring Hash LB Policy][A42] | ||
| * [A52: gRPC xDS Custom Load Balancer Configuration][A52] | ||
| * [A57: XdsClient Failure Mode Behavior][A57] |
There was a problem hiding this comment.
I would remove this from the list, since I don't think we actually need to reference it here.
| * [A92: xDS ExtAuthz Support][A92] | ||
| * [A93: xDS ExtProc Support][A93] |
There was a problem hiding this comment.
I don't think we need to reference A92 or A93 here.
| The converter for the `Slicer` xDS LB policy must set the `channel_factory_key` | ||
| field of the `slicer_experimental` LB policy to contain the same value that is | ||
| used as the map key in the returned map of GrpcServices. |
There was a problem hiding this comment.
I like this approach! This nicely avoids the need for the channel factory to transparently swap out the underlying channel when things like channel creds change.
3 participants
No description provided.