checker(dm): port free space lightning precheck #7733

lance6716 · 2022-11-28T10:17:53Z

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

What problem does this PR solve?

Issue Number: ref #3510

What is changed and how it works?

get source table size by SELECT data_length + index_length FROM information_schema.TABLES WHERE TABLE_SCHEMA = ? AND TABLE_NAME = ? and use it in lightning prechecks

Check List

Tests

Integration test

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

`None`.

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

codecov-commenter · 2022-11-28T12:36:30Z

Codecov Report

Merging #7733 (0221915) into master (6d24a57) will decrease coverage by 0.0113%.
The diff coverage is 63.3655%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`66.3831% <79.5811%> (+0.0812%)`	⬆️
dm	`52.0045% <41.2820%> (-0.1808%)`	⬇️
engine	`64.3079% <60.9319%> (+0.3111%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #7733        +/-   ##
================================================
- Coverage   59.9034%   59.8921%   -0.0114%     
================================================
  Files           810        811         +1     
  Lines         93255      93675       +420     
================================================
+ Hits          55863      56104       +241     
- Misses        32535      32696       +161     
- Partials       4857       4875        +18

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

lance6716 · 2022-11-29T09:55:32Z

/run-dm-integration-test

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

lance6716 · 2022-11-29T10:29:00Z

dm/checker/checker.go

+	if _, ok := c.checkingItems[config.LightningFreeSpaceChecking]; ok &&
+		c.stCfgs[0].LoaderConfig.ImportMode == config.LoadModePhysical &&
+		c.stCfgs[0].Mode != config.ModeIncrement {
+		// TODO: concurrently read it intra-source later


this PR is a bit large, I prefer fix it in next PR

D3Hunter

rest lgtm

dm/config/checking_item.go

dm/pkg/checker/lightning.go

dm/checker/checker.go

D3Hunter · 2022-11-30T03:42:13Z

dm/checker/checker.go

+// members may have repeated meanings but have different data structure to satisfy different usages.
+type tablePairInfo struct {
+	// downstream table -> sourceID -> upstream tables
+	tablesPerDownstreamTable map[filter.Table]map[string][]filter.Table


i think PerXXX naming is quite hard to know what's inside, maybe targetTable2SourceTableMap, below too

there's two name type: source/target and upstream/downstream. Do you prefer we always use source/target?

prefer source/target, they have less chars 😅, upstream/downstream are ok too

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

D3Hunter · 2022-11-30T06:45:07Z

dm/config/checking_item_test.go

 	}
-	c.Assert(ValidateCheckingItem("xxx"), NotNil)
+	// remember to update the number when add new checking items.
+	require.Equal(t, 5, lightningCheck)


this doesn't check LightningPrechecks ⊆ AllCheckingItems

D3Hunter · 2022-11-30T06:47:36Z

dm/checker/checker.go

+// members may have repeated meanings but have different data structure to satisfy different usages.
+type tablePairInfo struct {
+	// downstream table -> sourceID -> upstream tables
+	tablesPerDownstreamTable map[filter.Table]map[string][]filter.Table


prefer source/target, they have less chars 😅, upstream/downstream are ok too

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

ti-chi-bot · 2022-11-30T07:18:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
buchuitoudegou

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.

buchuitoudegou

rest lgtm

buchuitoudegou · 2022-12-01T03:10:56Z

dm/config/task.go

@@ -326,6 +329,10 @@ func (m *LoaderConfig) adjust() error {
 		m.PoolSize = defaultPoolSize
 	}

+	if m.Dir != "" && m.SortingDirPhysical == "" {
+		m.SortingDirPhysical = m.Dir


Why put the sorting-dir with source-dir? Lightning will prefer they are in different disk. Maybe we can set another default sorting-dir?

I don't know how to do it in code 😂 and user may be unexpected to see another disk is occupied

👌 Maybe just leave some comments to demonstrate the risk here?

there will be a precheck as I port more lightning precheck in future

dm/pkg/checker/lightning.go

Co-authored-by: Obliviate <756541536@qq.com>

lance6716 · 2022-12-01T07:22:39Z

/hold

I prefer we merge it after v6.5 branch is created

lance6716 · 2022-12-02T08:16:33Z

/unhold

/merge

ti-chi-bot · 2022-12-02T08:16:36Z

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

Commit hash: 5b157b9

ti-chi-bot · 2022-12-02T08:16:49Z

@lance6716: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

lance6716 · 2022-12-02T08:17:00Z

/run-all-tests

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

lance6716 · 2022-12-02T09:37:52Z

/merge

ti-chi-bot · 2022-12-02T09:37:55Z

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

Commit hash: 0105ac4

lance6716 · 2022-12-02T09:37:58Z

/run-all-tests

lance6716 added 2 commits November 28, 2022 18:16

checker(dm): port cluster free space lightning precheck

a03c28a

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

sync code

12bfa0f

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

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 28, 2022

lance6716 added 2 commits November 28, 2022 19:39

sync code

a870602

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

sync code

2df9ee6

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

lance6716 changed the title ~~[WIP]checker(dm): port cluster free space lightning precheck~~ checker(dm): port cluster free space lightning precheck Nov 28, 2022

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 28, 2022

refine comment

cbc51bf

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

lance6716 changed the title ~~checker(dm): port cluster free space lightning precheck~~ [WIP]checker(dm): port cluster free space lightning precheck Nov 28, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2022

lance6716 added 7 commits November 29, 2022 10:33

save work

48523f9

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

save work

30ff163

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

Merge branch 'master' of github.com:pingcap/tiflow into free-space

37353ec

sync code

18cc8f7

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

sync code

9b7e3b5

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

finish

8ba4a7f

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

fix UT

40942b2

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

reduce impact

6e063f0

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

lance6716 changed the title ~~[WIP]checker(dm): port cluster free space lightning precheck~~ checker(dm): port cluster free space lightning precheck Nov 29, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2022

lance6716 commented Nov 29, 2022

View reviewed changes

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 29, 2022

lance6716 requested review from buchuitoudegou and lichunzhu and removed request for buchuitoudegou and lichunzhu November 29, 2022 10:36

lance6716 requested a review from lichunzhu November 29, 2022 10:36

lance6716 changed the title ~~checker(dm): port cluster free space lightning precheck~~ checker(dm): port free space lightning precheck Nov 30, 2022

D3Hunter reviewed Nov 30, 2022

View reviewed changes

address some comments

0221915

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

D3Hunter reviewed Nov 30, 2022

View reviewed changes

address comment

c93258d

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

D3Hunter approved these changes Nov 30, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 30, 2022

buchuitoudegou reviewed Dec 1, 2022

View reviewed changes

Update dm/pkg/checker/lightning.go

5b157b9

Co-authored-by: Obliviate <756541536@qq.com>

buchuitoudegou approved these changes Dec 1, 2022

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 Dec 1, 2022

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 2, 2022

Merge branch 'master' into free-space

3866877

fix CI

0105ac4

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

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Dec 2, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 2, 2022

ti-chi-bot merged commit b953b42 into pingcap:master Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker(dm): port free space lightning precheck #7733

checker(dm): port free space lightning precheck #7733

lance6716 commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

lance6716 commented Nov 29, 2022

lance6716 Nov 29, 2022

D3Hunter left a comment

D3Hunter Nov 30, 2022

lance6716 Nov 30, 2022

D3Hunter Nov 30, 2022

D3Hunter Nov 30, 2022

D3Hunter Nov 30, 2022

ti-chi-bot commented Nov 30, 2022 •

edited

Loading

buchuitoudegou left a comment

buchuitoudegou Dec 1, 2022

lance6716 Dec 1, 2022

buchuitoudegou Dec 1, 2022

lance6716 Dec 1, 2022

lance6716 commented Dec 1, 2022

lance6716 commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

lance6716 commented Dec 2, 2022

lance6716 commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

lance6716 commented Dec 2, 2022

checker(dm): port free space lightning precheck #7733

checker(dm): port free space lightning precheck #7733

Conversation

lance6716 commented Nov 28, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

codecov-commenter commented Nov 28, 2022 • edited Loading

Codecov Report

lance6716 commented Nov 29, 2022

Choose a reason for hiding this comment

D3Hunter 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

ti-chi-bot commented Nov 30, 2022 • edited Loading

buchuitoudegou 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

lance6716 commented Dec 1, 2022

lance6716 commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

lance6716 commented Dec 2, 2022

lance6716 commented Dec 2, 2022

ti-chi-bot commented Dec 2, 2022

lance6716 commented Dec 2, 2022

lance6716 commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

ti-chi-bot commented Nov 30, 2022 •

edited

Loading