backupccl: retry restore's AdminSplit on ephemeral errors #85333
Conversation
Informs cockroachdb#84635,cockroachdb#84162 Release note: none
|
This change is |
| if err == nil { | ||
| break | ||
| } | ||
| err := s.db.AdminSplit(ctx, newSplitKey, expirationTime) |
There was a problem hiding this comment.
Is it worth moving this retry logic one level deeper? Maybe we want to ask KV but don't we have a lot of callers outside restore that use db.AdminSplit as well? import, index backfills etc.
There was a problem hiding this comment.
Or atleast auditing all the places we call db.AdminSplit and deciding whether to switch them to this wrapper method.
There was a problem hiding this comment.
Good point. I don't see why the caller needs to handle these replication level ephemeral retries. Lemme ask kv what they think in slack.
| } | ||
| err := s.db.AdminSplit(ctx, newSplitKey, expirationTime) | ||
|
|
||
| if !(kvserver.IsRetriableReplicationChangeError(err)) { |
There was a problem hiding this comment.
We might need to add IsLeaseTransferRejectedBecauseTargetMayNeedSnapshotError() here or inside some lower level e.g. IsRetriableReplicationChangeError() as this error is what #84891 is failing with.
|
Closing, as kvserver will handle this internally db2d653 |
Informs #84635,#84162
Release note: none