lightning: continue this region round and retry on next round when TiKV is busy #40278

lance6716 · 2023-01-03T08:47:06Z

Signed-off-by: lance6716 lance6716@gmail.com

What problem does this PR solve?

Issue Number: close #40205

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2023-01-03T08:47:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

buchuitoudegou
gozssky

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-03T09:07:31Z

/cc @gozssky @lichunzhu

lance6716 · 2023-01-03T09:08:02Z

/run-integration-br-tests

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-03T09:40:56Z

/run-integration-br-tests

br/pkg/lightning/backend/local/local.go

sleepymole · 2023-01-03T09:43:49Z

br/pkg/lightning/backend/local/local.go

+				confVer: r.Region.GetRegionEpoch().GetConfVer(),
+				version: r.Region.GetRegionEpoch().GetVersion(),
+			}
+			if _, ok := writeCheckpoint[checkpointKey]; !ok {


How to ensure the range bounds of nextRoundRegions are not changed after rescan?

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-04T08:25:28Z

/run-integration-br-tests

lance6716 · 2023-01-04T08:35:37Z

/retest

lance6716 · 2023-01-04T09:02:35Z

strange error if I make write to tikv no leader retryable 🤔

lance6716 · 2023-01-04T09:13:30Z

/retest

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-04T09:45:54Z

/run-integration-br-tests

lance6716 · 2023-01-04T10:21:09Z

/run-integration-br-tests

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-05T05:53:23Z

/run-integration-br-tests

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-05T06:50:59Z

/run-integration-br-tests

lance6716 · 2023-01-09T02:06:08Z

ptal @gozssky @lichunzhu

CI should be OK now

lance6716 · 2023-01-10T02:48:29Z

ptal @gozssky

br/pkg/lightning/backend/local/local.go

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-10T03:45:22Z

/run-integration-br-tests

lance6716 · 2023-01-10T05:37:55Z

/cc @D3Hunter @buchuitoudegou

buchuitoudegou

rest lgtm

buchuitoudegou · 2023-01-10T07:38:08Z

br/pkg/lightning/backend/local/local.go

+				case retryBusyIngest:
+					log.FromContext(ctx).Warn("meet tikv busy when ingest", log.ShortError(err), logutil.SSTMetas(ingestMetas),
+						logutil.Region(region.Region))
+					// ImportEngine will continue on this unfinished range


tidb/br/pkg/lightning/backend/local/local.go

Lines 1651 to 1682 in 3cb091b

for {

unfinishedRanges := lf.unfinishedRanges(ranges)

if len(unfinishedRanges) == 0 {

break

}

log.FromContext(ctx).Info("import engine unfinished ranges", zap.Int("count", len(unfinishedRanges)))

// if all the kv can fit in one region, skip split regions. TiDB will split one region for

// the table when table is created.

needSplit := len(unfinishedRanges) > 1 || lfTotalSize > regionSplitSize || lfLength > regionSplitKeys

// split region by given ranges

for i := 0; i < maxRetryTimes; i++ {

err = local.SplitAndScatterRegionInBatches(ctx, unfinishedRanges, lf.tableInfo, needSplit, regionSplitSize, maxBatchSplitRanges)

if err == nil || common.IsContextCanceledError(err) {

break

}

log.FromContext(ctx).Warn("split and scatter failed in retry", zap.Stringer("uuid", engineUUID),

log.ShortError(err), zap.Int("retry", i))

}

if err != nil {

log.FromContext(ctx).Error("split & scatter ranges failed", zap.Stringer("uuid", engineUUID), log.ShortError(err))

return err

}

// start to write to kv and ingest

err = local.writeAndIngestByRanges(ctx, lf, unfinishedRanges, regionSplitSize, regionSplitKeys)

if err != nil {

log.FromContext(ctx).Error("write and ingest engine failed", log.ShortError(err))

return err

}

}

Seems there is no retry limit for ImportEngine. Are there any risks that will make lightning hang/run too long?

I'm not sure about the old logic, PTAL @gozssky

Yes, there is no retry limit. Maybe we can check if there is progress on each retry. If there is no progress, then set a retry limit.

Don't want to add another retry counter and sleep interval for now. I'm thinking of providing a retry framework 😂 since the problem does not become worse than before, I prefer we do it later

ptal @buchuitoudegou

lance6716 · 2023-01-12T04:11:22Z

/run-integration-br-tests

lance6716 · 2023-01-12T06:08:51Z

/run-integration-br-tests

lance6716 · 2023-01-12T07:15:29Z

/run-integration-br-tests

lance6716 · 2023-01-12T07:22:47Z

/merge

BR CI is very unstable, will check the effects of this PR later

ti-chi-bot · 2023-01-12T07:22:51Z

This pull request has been accepted and is ready to merge.

Commit hash: d5c259d

finish

63a545b

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added release-note-none size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 3, 2023

fix some comments

6b9795a

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot requested review from sleepymole and lichunzhu January 3, 2023 09:07

fix CI

55a4ec0

Signed-off-by: lance6716 <lance6716@gmail.com>

sleepymole reviewed Jan 3, 2023

View reviewed changes

address comment

9e83b2e

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 4, 2023

fix errdoc

5d388b2

Signed-off-by: lance6716 <lance6716@gmail.com>

revert a change

64c006f

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' into add-retry

5ff28b4

add lightning debug log

cea4228

Signed-off-by: lance6716 <lance6716@gmail.com>

fix bug

0a64f26

Signed-off-by: lance6716 <lance6716@gmail.com>

sleepymole approved these changes Jan 10, 2023

View reviewed changes

br/pkg/lightning/backend/local/local.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 10, 2023

address comment

07f4fb9

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot requested review from buchuitoudegou and D3Hunter January 10, 2023 05:37

buchuitoudegou reviewed Jan 10, 2023

View reviewed changes

buchuitoudegou approved these changes Jan 12, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 12, 2023

Merge branch 'master' into add-retry

e2b557b

Merge branch 'master' into add-retry

d5c259d

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2023

ti-chi-bot merged commit aef752a into pingcap:master Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: continue this region round and retry on next round when TiKV is busy #40278

lightning: continue this region round and retry on next round when TiKV is busy #40278

lance6716 commented Jan 3, 2023

ti-chi-bot commented Jan 3, 2023 •

edited

lance6716 commented Jan 3, 2023

lance6716 commented Jan 3, 2023

lance6716 commented Jan 3, 2023

sleepymole Jan 3, 2023 •

edited

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 5, 2023

lance6716 commented Jan 5, 2023

lance6716 commented Jan 9, 2023

lance6716 commented Jan 10, 2023

lance6716 commented Jan 10, 2023

lance6716 commented Jan 10, 2023

buchuitoudegou left a comment

buchuitoudegou Jan 10, 2023

lance6716 Jan 10, 2023

sleepymole Jan 10, 2023

lance6716 Jan 10, 2023

lance6716 Jan 11, 2023

buchuitoudegou Jan 12, 2023

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

	for {
	unfinishedRanges := lf.unfinishedRanges(ranges)
	if len(unfinishedRanges) == 0 {
	break
	}
	log.FromContext(ctx).Info("import engine unfinished ranges", zap.Int("count", len(unfinishedRanges)))

	// if all the kv can fit in one region, skip split regions. TiDB will split one region for
	// the table when table is created.
	needSplit := len(unfinishedRanges) > 1 \|\| lfTotalSize > regionSplitSize \|\| lfLength > regionSplitKeys
	// split region by given ranges
	for i := 0; i < maxRetryTimes; i++ {
	err = local.SplitAndScatterRegionInBatches(ctx, unfinishedRanges, lf.tableInfo, needSplit, regionSplitSize, maxBatchSplitRanges)
	if err == nil \|\| common.IsContextCanceledError(err) {
	break
	}

	log.FromContext(ctx).Warn("split and scatter failed in retry", zap.Stringer("uuid", engineUUID),
	log.ShortError(err), zap.Int("retry", i))
	}
	if err != nil {
	log.FromContext(ctx).Error("split & scatter ranges failed", zap.Stringer("uuid", engineUUID), log.ShortError(err))
	return err
	}

	// start to write to kv and ingest
	err = local.writeAndIngestByRanges(ctx, lf, unfinishedRanges, regionSplitSize, regionSplitKeys)
	if err != nil {
	log.FromContext(ctx).Error("write and ingest engine failed", log.ShortError(err))
	return err
	}
	}

lightning: continue this region round and retry on next round when TiKV is busy #40278

lightning: continue this region round and retry on next round when TiKV is busy #40278

Conversation

lance6716 commented Jan 3, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 3, 2023 • edited

lance6716 commented Jan 3, 2023

lance6716 commented Jan 3, 2023

lance6716 commented Jan 3, 2023

sleepymole Jan 3, 2023 • edited

Choose a reason for hiding this comment

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 4, 2023

lance6716 commented Jan 5, 2023

lance6716 commented Jan 5, 2023

lance6716 commented Jan 9, 2023

lance6716 commented Jan 10, 2023

lance6716 commented Jan 10, 2023

lance6716 commented Jan 10, 2023

buchuitoudegou left a comment

Choose a reason for hiding this comment

buchuitoudegou Jan 10, 2023

Choose a reason for hiding this comment

lance6716 Jan 10, 2023

Choose a reason for hiding this comment

sleepymole Jan 10, 2023

Choose a reason for hiding this comment

lance6716 Jan 10, 2023

Choose a reason for hiding this comment

lance6716 Jan 11, 2023

Choose a reason for hiding this comment

buchuitoudegou Jan 12, 2023

Choose a reason for hiding this comment

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

lance6716 commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

ti-chi-bot commented Jan 3, 2023 •

edited

sleepymole Jan 3, 2023 •

edited