vmg
commented
3 years ago More thoughts on proactively detecting re-sharding operations and failovers:
As explained on the previous comment, right now there's a shardBuffer data structure that is lazily allocated for every keyspace+shard pair on first access, and whose buffering logic is only activated the first time there's an error in the destination shard. This design doesn't seem feasible once we lift this logic higher in the stack.
I think the solution we're looking for is a global map (for each vtgate) of "cluster events" that is updated in real time. Each event would contain information about what keyspaces and shards are affected, and then we'd just use auxiliary data structures so that when a query plan comes in, we could efficiently detect whether any of the shards where the plan would be executed are currently affected by any events, and if so, so we could buffer the plan before attempting it. Likewise, the "event" itself would keep a goroutine actively polling the health of the cluster so that it can detect its own resolution, and when that happens, it would flush all the currently buffered requests that were waiting on the event.
Does this seem reasonable? I think the hardest problem about this is the case where we don't proactively detect an event in the cluster, and instead we receive a failover error from one of the tablets. Converting this tablet (and hence shard-) specific error into a global event that could potentially affect more than one shard seems complicated. How would we handle this?
deepthi
harshit-gangal
rafael
I have started working on this functionality for Vitess. To ellaborate on the problem, which is already listed in https://github.com/vitessio/vitess/issues/7061: we want to improve the way we perform buffering to be able to handle more complex resharding situations.
Currently, the main entrypoint for buffering is in
tabletgateway.go, in theTabletGateway.withRetryhelper that wraps all the method calls for ourQueryService:https://github.com/vitessio/vitess/blob/4bebb8e5a6097772bcb8a4d1c187f14782158069/go/vt/vtgate/tabletgateway.go#L216-L234
Here's a few important things to note:
DiscoveryGateway, but this whole API has been deprecated in favor ofTabletGateway, so we can ignore it. :ok_hand:mastertabletLet's look at the two critical issues which we intend to fix:
The way buffering works right now is by keeping a table of "buffers" for every keyspace and shard pair. This "shard buffer" is created lazily the first time a query for a given keyspace+shard arrives to the
vtgate, and by default it is disabled, meaning that requests pass through directly and are not buffered. The first time that a request to a given keyspace/shard returns an error, the retry code inTabletGatewaywill retry the buffering before retrying the request, this time passing the upstream error. If the buffering code detects that the upstream error is a failover error (this is explicitly checked at https://github.com/vitessio/vitess/blob/30250f51afd98dc1629b233959b60a916d88f7ff/go/vt/vtgate/buffer/buffer.go#L263-L290), then the buffer for the keyspace+shard will be marked as "enabled", and the current query will start buffering. Any subsequent queries that target the same keyspace+shard will also start buffering once they reach the buffering check, because their buffer is now marked as "enabled".This is what we mean by "lazy buffering": the only way a request will start buffering is once it has failed at least once for its keyspace+shard. The retry for this request, and any follow up requests, will then buffer. In an ideal design for this system, the buffering code will be proactively health checking the different shards in the cluster so that we can buffer a request right away once we detect a failover, without having to use an error response as the detection mechanism for the failover.
The other problem with this design is caused by the scoping to keyspace+shard which we've already discussed: this works fine for a master failover in a given shard, but fails critically during re-sharding operations. When we're performing a cutover after re-sharding a Vitess cluster, we're not only handling a master-slave failover for a specific shard, we're potentially changing the number of shards in the cluster and their respective masters. There is no clear way to detect once the failover is finished in a specific shard because the re-sharding operation necessarily affects more than one shard, and the resulting topology for our cluster can potentially force us to retry the buffered query in a different shard than the one we had initially planned before we started buffering.
The suggested fix
The fix that has been agreed on in our discussions so far implies lifting the buffering code so it happens way before the current checks in
TabletGateway, and consequently, so it is not specifically scoped to a keyspace+shard pair.A suggested location to move our buffering logic would be in the
Executorfor thevtgates, roughly after the initial planning has succeeded, but before the plan is executed:https://github.com/vitessio/vitess/blob/30250f51afd98dc1629b233959b60a916d88f7ff/go/vt/vtgate/plan_execute.go#L52-L82
The idea would be providing an API that takes the suggested plan and verifies whether the shards it affects need buffering (i.e. because they're being re-sharded or in the middle of a failover). If that's the case, we'd buffer the request until we detect that the whole re-sharding operation is complete and then we would retry the planning to ensure the final plan we execute is updated based on the cluster's topology after resharding.
This design assumes that we can implement a pro-active API for the buffering code that can tell us whether a given plan needs buffering before execution and, ideally, also whether the given plan would need to be re-planned after the buffering because it's been affected by a topology change. This second part is not mandatory for correctness, but feels like an important optimization for V2 -- for V1 of this implementation, we can assume that any plan that has been buffered before executing always needs to be re-planned, which is safe to assume.
However: this raises an issue with actual upstream retries. Right now this code is implemented in the
withRetryhelper forTabletGatewaywhich we've already discussed, and as part of the retrying logic, includes the potential buffering when the error being retried is detected as a failover error. What happens with this retry code once we lift the buffering logic out of it?My assumption is that
TabletGatewayneeds to keep its retry logic (mostly because this logic includes the shuffling for the different tablets in a shard so they can be retried when one of them is down), but now we also need to add retrying logic higher in the stack (i.e. where we're now buffering), so that we can enable the buffering based on the return error in cases where we didn't detect a re-sharding or failover proactively. Does this sound correct? Will this extra retry logic introduce any unforseen consequences? For starters, it seems like any failover-related errors inTabletGatewayshould be changed to be bubbled up immediately and not retried, but are there any other changes that need to be introduced?@sougou @harshit-gangal @rohit-nayak-ps: this is everything I understand about the problem so far. Could you please read through this and correct me where I'm wrong? I'll keep updating the issue in follow-up comments as I research further.