workloadrepo: try to recover etcd snapID from table #59628
Conversation
ti-chi-bot
bot
added
release-note-none
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/L
|
Hi @xhebox. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: xhe <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #59628 +/- ##
================================================
+ Coverage 72.9595% 73.3481% +0.3885%
================================================
Files 1694 1695 +1
Lines 468654 469217 +563
================================================
+ Hits 341928 344162 +2234
+ Misses 105675 104005 -1670
+ Partials 21051 21050 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| snapID, err = w.getSnapID(ctx) | ||
| if stderrors.Is(err, errKeyNotFound) { |
There was a problem hiding this comment.
Please add some comments on when this may happen.
| return err == nil && snapID > 0 | ||
| }, time.Minute, time.Second) | ||
| worker.stop() | ||
|
|
There was a problem hiding this comment.
We should probably wait for the worker to stop.
| // wait for worker to stop | |
| require.Eventually(t, func() bool { | |
| return worker.cancel == nil | |
| }, time.Second*10, time.Millisecond*100) |
| snapID, err := strconv.ParseUint(res[0][0].(string), 10, 64) | ||
| prevSnapID = snapID | ||
| return err == nil && snapID > 0 | ||
| }, time.Minute, time.Second) |
There was a problem hiding this comment.
Maybe time.Second should be time.Millisecond*100?
| require.EqualError(t, errKeyNotFound, err.Error()) | ||
| newSnapID, err := queryMaxSnapID(ctx, worker2.getSessionWithRetry().(sessionctx.Context)) | ||
| require.Nil(t, err) | ||
| require.Equal(t, prevSnapID, newSnapID) |
There was a problem hiding this comment.
This code assumes that prevSnapID has to match the last run. This based on timing. It might be better to just run a SQL query to validate that this value is one greater than max(snap_id).
| require.Eventually(t, func() bool { | ||
| newSnapID, err = worker2.getSnapID(ctx) | ||
| return err == nil && newSnapID >= prevSnapID | ||
| }, time.Minute, time.Second) |
There was a problem hiding this comment.
I would retry every 100 milliseconds instead.
What problem does this PR solve?
Issue Number: ref #58247
Problem Summary: Serverless TiDB wants a specialized auto-scalable etcd cluster for etcd usages in TiDB, rather than relying on PD. However, moving from PD to the new etcd cluster will also remove all persistent data on PD. We can workaround this by recovering snapID from table.
Also check tidbcloud/tidb-cse#1522
What changed and how does it work?
As title.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.