Lightning: optimize the estimated size #41943

okJiang · 2023-03-06T03:13:18Z

What problem does this PR solve?

Issue Number: close #41942

Problem Summary:

What is changed and how it works?

The compression ratio for TiKV is typically around 3 to 1.

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

ti-chi-bot · 2023-03-06T03:13:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dsdashun
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.

okJiang · 2023-03-06T03:44:44Z

/check-issue-triage-complete

…te-size

br/pkg/lightning/restore/get_pre_info.go

br/pkg/lightning/restore/precheck_impl.go

dsdashun · 2023-03-06T06:05:21Z

br/pkg/lightning/restore/restore.go

@@ -115,7 +115,8 @@ const (
 		status  VARCHAR(32) NOT NULL,
 		state   TINYINT(1) NOT NULL DEFAULT 0 COMMENT '0: normal, 1: exited before finish',
 		source_bytes BIGINT(20) UNSIGNED NOT NULL DEFAULT 0,
-		cluster_avail BIGINT(20) UNSIGNED NOT NULL DEFAULT 0,


For existing clutters，this table has already been created, and the table will only be created if the table doesn't exist. This means the table schema will remain the old one without those two new fields for those existing clusters.

Lightning is once-only used. It is fine?

dsdashun · 2023-03-06T06:08:11Z

br/pkg/lightning/restore/get_pre_info.go

@@ -561,6 +561,10 @@ func (p *PreRestoreInfoGetterImpl) EstimateSourceDataSize(ctx context.Context, o
 		}
 	}

+	if !isTiDBBackend(p.cfg) {
+		sizeWithIndex = sizeWithIndex / 3


Can you add a comment here describing why size should be divided by 3 ?

dsdashun · 2023-03-06T06:35:16Z

br/pkg/lightning/restore/precheck_impl.go

@@ -153,13 +162,23 @@ func (ci *clusterResourceCheckItem) Check(ctx context.Context) (*CheckResult, er
 		return nil, errors.Trace(err)
 	}
 	estimateSize := clusterSource * replicaCount


Here, the estimated size is multiplied by the number of replicas. However, you have divided the available size of the entire cluster into TiKV size and TiFlash size. There may be false alarms. For example, suppose there's only one TiFlash node in a cluster, and it is sufficient to store one replica. However, the check indicates that this node is not large enough to store multiple replicas. In this situation, the check should pass IMO.

After 263bba7 and a898ff6, The Tiflash check should be same as TiKV, I think it be able to effect the result.

br/pkg/lightning/restore/get_pre_info.go

br/pkg/lightning/restore/precheck_impl.go

okJiang · 2023-03-06T10:05:59Z

br/pkg/lightning/restore/get_pre_info.go

@@ -54,6 +54,9 @@ import (
 	"golang.org/x/exp/maps"
 )

+// compressionRatio is the tikv/tiflash's compression ratio
+const compressionRatio = float64(1) / 3


After asked tiflash member, the size in tikv is similar with tiflash for one replica

…te-size

sleepymole

Rest LGTM.

sleepymole · 2023-03-07T08:29:28Z

br/pkg/lightning/importer/get_pre_info.go

-					} else {
-						// if sample data failed due to max-error, fallback to use source size
-						sizeWithIndex += tbl.TotalSize
+						tableSize = int64(float64(tbl.TotalSize) * tbl.IndexRatio)


Can we remove the if tbl.IndexRatio > 0? It seems tbl.IndexRatio can not be zero.

It is removed in 931f006

sleepymole · 2023-03-07T08:37:32Z

br/pkg/lightning/importer/precheck_impl.go

-	estimateSize := clusterSource * replicaCount
-	if estimateSize > clusterAvail {
+	estimateTikvSize := tikvSourceSize * replicaCount
+	// note: tiflashSourceSize contains replicaCount


Can we also let tikvSourceSize contains replicaCount?

yes, we can 931f006

sleepymole · 2023-03-07T08:40:07Z

br/pkg/lightning/importer/table_import_test.go

@@ -99,6 +99,10 @@ func (s *tableRestoreSuiteBase) setupSuite(t *testing.T) {
 	core, err := ddl.MockTableInfo(se, node.(*ast.CreateTableStmt), 0xabcdef)
 	require.NoError(t, err)
 	core.State = model.StatePublic
+	core.TiFlashReplica = &model.TiFlashReplicaInfo{


Please add a test case for multiple tables with different tiflash replicas.

added in 931f006

okJiang · 2023-03-09T03:45:40Z

ptal again~ @dsdashun @gozssky

ti-chi-bot · 2023-03-09T14:38:11Z

@yabola: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

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 ti-community-infra/tichi repository.

; branch 'master' of https://github.com/pingcap/tidb into optimize-estimate-size

sleepymole · 2023-03-13T02:31:07Z

@okJiang Did you do any tests on real clusters?

okJiang · 2023-03-13T06:44:40Z

@okJiang Did you do any tests on real clusters?

Yes, but more detailed testing is underway.

resp="{"check_results":[{"check_item":"CHECK_ITEM_TARGET_CLUSTER_SIZE", "has_passed":false, "msg":"TiKV requires more storage space. Estimated required size: 312.6GiB. Actual size: 271.6GiB. Please increase storage to prevent import task failures."}

okJiang · 2023-03-14T06:04:34Z

/merge

ti-chi-bot · 2023-03-14T06:04:38Z

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

Commit hash: 309a05d

optimize estimating size

bc8f228

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2023

optimize the error message

aacafd1

ti-chi-bot removed the do-not-merge/needs-triage-completed label Mar 6, 2023

Merge branch 'master' of github.com:pingcap/tidb into optimize-estima…

204f3c4

…te-size

sleepymole added the component/lightning This issue is related to Lightning of TiDB. label Mar 6, 2023

sleepymole requested review from dsdashun and sleepymole March 6, 2023 06:07

sleepymole reviewed Mar 6, 2023

View reviewed changes

br/pkg/lightning/restore/get_pre_info.go Outdated Show resolved Hide resolved

br/pkg/lightning/restore/precheck_impl.go Outdated Show resolved Hide resolved

dsdashun reviewed Mar 6, 2023

View reviewed changes

okJiang added 2 commits March 6, 2023 14:40

fix ci

2b3c8f4

fix comment: add estimated size for tiflash

263bba7

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2023

okJiang added 3 commits March 6, 2023 15:57

add compressionRatio for tiflash

a898ff6

fix ci

06c50e9

fix ci

095e07f

sleepymole reviewed Mar 6, 2023

View reviewed changes

br/pkg/lightning/restore/get_pre_info.go Outdated Show resolved Hide resolved

br/pkg/lightning/restore/precheck_impl.go Outdated Show resolved Hide resolved

br/pkg/lightning/restore/precheck_impl.go Outdated Show resolved Hide resolved

okJiang added 3 commits March 6, 2023 17:55

fix ci

b10e914

exchange the order

2da4928

fix an typo

7f298bd

okJiang commented Mar 6, 2023

View reviewed changes

okJiang added 2 commits March 6, 2023 18:23

fix unit test

dcc7d8a

Merge branch 'master' of github.com:pingcap/tidb into optimize-estima…

fe14269

…te-size

sleepymole reviewed Mar 7, 2023

View reviewed changes

ti-chi-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 8, 2023

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 8, 2023

fix comment

931f006

okJiang force-pushed the optimize-estimate-size branch from 7c637cf to 931f006 Compare March 8, 2023 07:19

okJiang added 2 commits March 8, 2023 16:15

refine message

c84924c

fix message unit test

504942e

yabola approved these changes Mar 9, 2023

View reviewed changes

Merge branch 'optimize-estimate-size' of https://github.com/okJiang/tidb

ed3347d

; branch 'master' of https://github.com/pingcap/tidb into optimize-estimate-size

sleepymole approved these changes Mar 13, 2023

View reviewed changes

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

fix comment

309a05d

dsdashun approved these changes Mar 14, 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 Mar 14, 2023

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

okJiang merged commit 4ac0120 into pingcap:master Mar 14, 2023

sleepymole mentioned this pull request Mar 14, 2023

lightning integration test fail #42214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightning: optimize the estimated size #41943

Lightning: optimize the estimated size #41943

okJiang commented Mar 6, 2023

ti-chi-bot commented Mar 6, 2023 •

edited

Loading

okJiang commented Mar 6, 2023

dsdashun Mar 6, 2023

okJiang Mar 6, 2023

dsdashun Mar 6, 2023

dsdashun Mar 6, 2023

okJiang Mar 6, 2023

okJiang Mar 6, 2023 •

edited

Loading

sleepymole left a comment

sleepymole Mar 7, 2023

okJiang Mar 8, 2023

sleepymole Mar 7, 2023

okJiang Mar 8, 2023

sleepymole Mar 7, 2023

okJiang Mar 8, 2023

okJiang commented Mar 9, 2023

ti-chi-bot commented Mar 9, 2023

sleepymole commented Mar 13, 2023

okJiang commented Mar 13, 2023

okJiang commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

Lightning: optimize the estimated size #41943

Lightning: optimize the estimated size #41943

Conversation

okJiang commented Mar 6, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 6, 2023 • edited Loading

okJiang commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

sleepymole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang commented Mar 9, 2023

ti-chi-bot commented Mar 9, 2023

sleepymole commented Mar 13, 2023

okJiang commented Mar 13, 2023

okJiang commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

ti-chi-bot commented Mar 6, 2023 •

edited

Loading

okJiang Mar 6, 2023 •

edited

Loading