[TNTP-8909] fix: stop following the transfer destination on bucket ref error retries - #511
Conversation
077fa83 to
e8ec406
Compare
e8ec406 to
0b500ca
Compare
3d130e0 to
4818995
Compare
4818995 to
7d134de
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Almost there! We're moving fast
Please, let's properly split the changes between the commits. Otherwise, I cannot judge, whether the code is properly splitted and I am forced to look at all the changes in their entirety, this way it's way easier to skip the bug.
And if you don't mind, can you in the future please write your responses to each comment before re-requesting their review? This helps a lot to see if you have addressed the comments. Even a simple "Fixed" is better than nothing. Thanks in advance!
1c9c2f2 to
ff41e1a
Compare
e6320e0 to
91d8a35
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Looks like the last iteration to me. Thank you for putting up with my whining :)
44dce5c to
1ebb427
Compare
e9abc48 to
ced2b4c
Compare
wrap_vshard_err() took a bucket_id and, when the replicaset id was not known, resolved it by routing the bucket and then matching the replicaset object against vshard_router:routeall() -- a workaround for tarantool/vshard#460 for the versions where a replicaset had no id field. The rockspec now requires vshard >= 0.1.41, where replicaset.id is always available, so the workaround is obsolete. Now the replicaset id is always passed by the caller and vshard_utils.get_replicaset_id() is dropped;
0714738 to
2ef416c
Compare
|
I also returned back recovery for call.map because we actually can handle MISSING_MASTER error in it. |
2ef416c to
2a967f4
Compare
The _bucket trigger that turns the rebalance safe mode on recognized the start of a bucket migration by two markers: an INSERT of a bucket in the RECEIVING status and a REPLACE of a bucket into the SENDING status. Since vshard 0.1.41 a transfer on the source storage starts with an UPDATE to the READONLY status instead, so neither marker fired and the safe mode stayed disabled for the whole rebalancing. Handle the UPDATE to READONLY as one more start marker. The REPLACE to SENDING branch is kept for compatibility with older vshard versions. Part of #509.
4a05ba9 to
156f935
Compare
call_with_retry_and_recovery() retried every failed call once, whatever the error was, and for a single call it followed the destination reported in a bucket ref error. The destination may not have received the bucket yet, so in fast mode the request was applied there against an empty space and the same key could end up on two storages. An unrelated failure -- a storage error, a timeout -- was retried just as blindly, silently doubling the requested timeout. Recovery becomes an explicit step: recover_from_err() performs an action for the errors it knows and only then reports whether a retry makes sense. The master is located on MISSING_MASTER and the cached one is updated on NON_MASTER; on WRONG_BUCKET, BUCKET_IS_LOCKED and TRANSFER_IS_IN_PROGRESS the route cache is reset and the target is re-discovered via vshard:route() instead of being taken from the error. If the action did not help, or the error is anything else, it is returned to the caller as is. A deadline is taken before the first attempt, so the retry never runs past the requested timeout. Map calls recover from MISSING_MASTER only: vshard does not search for a master on an async call and reports it right away, while storage errors arrive later, in the future payload -- see #513. On the test side, the safe mode setup is extracted from helpers.start_cluster() into helpers.set_safe_mode(), so that a test can toggle the mode on a running cluster. Closes #509.
156f935 to
4ffc828
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Thank you for the fixes, the current version seems correct. Speaking of existing code, it requires substantial refactoring, it's very hard to make changes to it now: there're too many rough edges and small bugs in it. But I really appreciate, that you've fixed the things we've found.
I really hope, that someday we'll drop all that code from crud and allow vshard to do its work in order to avoid such nasty bugs. But for that to happen we must find a way to bump the perf of all requests
During rebalancing the source storage rejects a request with a bucket
ref error (e.g. TRANSFER_IS_IN_PROGRESS) and reports the transfer
destination. The router followed this redirect and retried the request
directly on the destination.
The destination, however, may not have received the bucket yet: it has
neither the _bucket record nor the data, and it is still in fast mode,
so no bucket ownership check is performed. The request was applied
against an empty space and acknowledged to the client. Once
bucket_recv() delivered the data, the cluster state diverged from the
acknowledged result: a deleted tuple reappeared, and the final result
of replace/update/upsert could differ from what was acknowledged.
The router no longer follows the destination reported by the error:
the bucket route cache is reset and the retry target is re-discovered
via vshard discovery, so a request is applied only on the replicaset
that actually holds the bucket.
A failed request is retried at most once and only after a recovery
action: the master is located on MISSING_MASTER, the cached master is
updated on NON_MASTER, the bucket route is reset on bucket ref errors
and the retry is re-routed by the bucket id. Other errors are returned
to the caller without a retry: the old code retried them blindly,
which silently doubled the effective request timeout. The retry is
given only the time left before the request deadline. Map requests are
not recovered at all: they are async, so storage errors arrive later,
in the future payload (see #513).
The rebalance safe mode also missed the start of a transfer on vshard
0.1.41+, where the bucket status is UPDATEd to READONLY instead of
being REPLACEd with SENDING, so safe mode was not enabled during
rebalancing and writes were not protected with bucket refs. Now the
READONLY update enables the safe mode as well.
Closes #509