A119: Auto-Sharding 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!
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This is getting closer!
Please let me know if you have any questions. Thanks!
| // A timeout value for fallback to kick in when no assignments have been | ||
| // received from the sharding service. | ||
| // Defaults to 60 seconds if not specified. | ||
| google.protobuf.Duration fallback_timeout = 6; |
There was a problem hiding this comment.
It doesn't really matter whether it's defined here or in A121. I guess it should be in whichever of the two gRFCs is going to be merged last.
Please coordinate with @mbissa.
| allEndpointsInSlice []int // Indices into sliceMap.allEndpoints | ||
| endpointsByState [5][]int // Array indexed directly by connectivity.State (ranges 0..4) |
There was a problem hiding this comment.
Okay. @pawbhard, let's consider this for the C-core implementation.
| 1. Parsing a `GrpcService` proto embedded within an LB policy's configuration | ||
| into its internal representation, requires access to the following: | ||
| * the complete bootstrap configuration to access the `allowed_grpc_services` | ||
| section of the bootstrap configuration. | ||
| * configuration of the specific xDS server that delivered this resource, to | ||
| determine if the server is to be trusted or not. |
|
@markdroth : I think I've addressed all your comments. Thanks for the detailed review. |
| If validation fails, the LB policy must terminate the stream to the sharding | ||
| service, and attempt to re-establish it. |
There was a problem hiding this comment.
Is this really the right thing to do? If we get an invalid assignment, that's likely a bug in the slicer service, and it's not clear to me that reestablishing the stream will fix it -- we'll probably just get the same bad data again after we restart the stream. But restarting the stream will cause load on the server, so I'd be worried about a cascading failure here: a bug in the slicer service could cause all of the clients to start terminating and reconnecting streams, which could cause a lot of unexpected load.
I suggest asking the slicer folks how they handle this kind of case today.
There was a problem hiding this comment.
I've updated this section. PTAL. Thanks.
There was a problem hiding this comment.
Also, updated to include a new component named AssignmentProvider which will encapsulate all details of the interaction with the sharding service and will provide usable assignments to the LB policy.
One thing I'm still a little uncertain is whether if one of those validations fail (like one slice entry contains an invalid index), do we still use the new assignment but send an AssignmentAck with accepted set to true but with the error message populated? (I currently have it the other way around, but willing to change it).
If we do the above, then the only cases where we would end up setting the accepted field to false would be when we have no usable slices in the assignment. Is that your understanding too?
Thanks
| If a new gRPC channel is created: | ||
| * Create a new `Shard` stream on the newly created gRPC channel, and, | ||
| * Close the previously created gRPC channel to the sharding service | ||
| * If the `slicing_target` field has changed, create a new `Shard` stream because |
There was a problem hiding this comment.
In this case do we discard existing and enter assignment pending ?
There was a problem hiding this comment.
No, we start a new initial assignment timer but keep using the previous assignment until we either get a new assignment or the timer fires. This is covered in the "Fallback at Startup" section above.
| #### Assignment | ||
|
|
||
| The LB policy relies on an internal helper component, which we will refer to as | ||
| the `AssignmentProvider`, to produce a validated and gap-free set of key-ranges |
There was a problem hiding this comment.
Consider calling this AutoshardingClient instead.
| If validation fails, the `AssignmentProvider` must not send an update to the LB | ||
| policy and must send an `AssignmentAck` message with the following contents: |
There was a problem hiding this comment.
If we get an update that is invalid, I think we should return an indication of an invalid update to the LB policy. That way, if the LB policy is waiting for the initial assignment timer, it can immediately act as if the timer has fired rather than waiting for the timeout. That way, we don't need to wait for the timer to fire before going into fallback mode or starting to fail RPCs.
| Note that gaps in the key-ranges represented by the `Slice`s are allowed. In | ||
| this case, the `AssignmentProvider` must fill these gaps with `Slice`s that | ||
| contain no endpoints. This will cause requests that match these `Slice`s to | ||
| fallback (if enabled) or fail. |
There was a problem hiding this comment.
I don't remember where we landed on this... Didn't we decide that if this happens, we should send back an AssignmentAck that sets the error_message but sets accepted to true?
I'm pretty sure there was some case where we were going to send back an error message with accepted = true, because that was the main reason we separated this into two fields.
| m: dict[str, EndpointState] | ||
| ``` | ||
|
|
||
| The LB policy must create a new `EndpointMap` whenever it receives new endpoints |
There was a problem hiding this comment.
We should probably say something here about how we handle collisions in hostnames. I think we should just say that implementations should pick one of them, but it doesn't matter which one.
| * This allows the sharding service to not resend a previously sent good | ||
| assignment in the case of a stream failure. | ||
|
|
||
| #### Handling responses from the Autosharding server |
There was a problem hiding this comment.
I think we need to cover the fact that if we restart the stream and receive an update with a generation number older than the last one we previously saw, we should ignore the old update and stick with the previous assignment.
However, if the channel key changes, do we want to retain the previous generation? I think we should not, because if the channel target changed, we might be talking to a completely different autosharding service that might have a completely different generation number. If we refuse to accept an assignment with a lower generation than the one that we saw from the previous autosharding service, we may fail to accept any updates for a long time.
Note that it is also possible that the channel key will change because of initial metadata or credentials changing without actually changing the channel target, in which case we don't really need to throw away the generation number. However, those cases seem less likely in practice, and the behavior is really not that bad if we do throw away the generation number in that case: we would temporarily revert to an older assignment, but presumably the problem would fix itself fairly quickly on the next update.
Similar question when the autosharding target changes. I assume that there will be a different generation number for each target, so we probably want to throw away the previously seen generation number in that case too.
| # No assignment covers this key. This is only possible when the | ||
| # initial_assignment_timeout has expired *and* no valid assignments have | ||
| # been received from the sharding service. | ||
| if slice_idx is None: |
There was a problem hiding this comment.
I think it might wind up being easier to return a separate failing picker in this case. In C-core, we have a common TransientFailurePicker class that any LB policy can return when it wants to fail all RPCs. If we do that, then we know that the slice map lookup here must always return a valid result, because we won't be using this picker in any case where it wouldn't.
I'm not sure if Java and Go have an equivalent of the TransientFailurePicker or not. If not, it's fine to leave this as-is -- maybe just add a note that implementations can choose to use a different picker for this case.
CC @pawbhard
|
|
||
| def pick(self, pick_args: PickArgs) -> PickResult: | ||
| # Extract sharding key from request metadata/header | ||
| key = extract_key_from_metadata(pick_args, self.lb_config.key_header_name) |
There was a problem hiding this comment.
If this lookup fails (i.e., if the RPC does not have the specified header), we should fail the pick.
5 participants
No description provided.