Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

importinto/lightning: do remote checksum via sql #44803

Merged
merged 27 commits into from
Jun 21, 2023
Merged

importinto/lightning: do remote checksum via sql #44803

merged 27 commits into from
Jun 21, 2023

Conversation

GMHDBJD
Copy link
Contributor

@GMHDBJD GMHDBJD commented Jun 19, 2023 •
edited by dsdashun
Loading

What problem does this PR solve?

Issue Number: close #41941 , ref #44711

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2023
niubell
niubell reviewed Jun 20, 2023
View reviewed changes
br/pkg/lightning/importer/import.go Outdated
manager, err := NewChecksumManager(ctx, rc, kvStore)
var manager local.ChecksumManager
if rc.cfg.TikvImporter.Backend == config.BackendLocal && rc.cfg.PostRestore.Checksum != config.OpLevelOff {
manager = local.NewTiDBChecksumExecutor(rc.db)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about add a hidden switch for checksum via sql or tikv ?

disttask/importinto/subtask_executor.go
@@ -73,6 +84,115 @@ func (e *postProcessMinimalTaskExecutor) Run(ctx context.Context) error {
return postProcess(ctx, mTask.taskMeta, &mTask.meta, mTask.logger)
}

// postProcess does the post-processing for the task.
func postProcess(ctx context.Context, taskMeta *TaskMeta, subtaskMeta *PostProcessStepMeta, logger *zap.Logger) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add more unit tests for this function or checksum via sql

D3Hunter
D3Hunter reviewed Jun 20, 2023
View reviewed changes
disttask/importinto/subtask_executor.go Outdated
zap.Int("concurrency", distSQLScanConcurrency), zap.Int("retry", i))
if distSQLScanConcurrency > local.MinDistSQLScanConcurrency {
distSQLScanConcurrency = mathutil.Max(distSQLScanConcurrency/2, local.MinDistSQLScanConcurrency)
se.GetSessionVars().SetDistSQLScanConcurrency(distSQLScanConcurrency)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert it back after execute? the session taken from resource pool

disttask/importinto/subtask_executor.go Outdated
distSQLScanConcurrency = se.GetSessionVars().DistSQLScanConcurrency()

for i := 0; i < maxErrorRetryCount; i++ {
rs, execErr = storage.ExecSQL(ctx, se, sql)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put in explicit txn?

@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2023
dsdashun
dsdashun reviewed Jun 20, 2023
View reviewed changes
br/pkg/checksum/executor.go Outdated
@@ -342,12 +342,14 @@ func (exec *Executor) Execute(
//
// It is useful in TiDB, however, it's a place holder in BR.
killed := uint32(0)
vars := kv.NewVariables(&killed)
vars.BackOffWeight *= 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it a configurable parameter ?

br/pkg/lightning/backend/local/checksum.go Outdated
@@ -111,7 +113,11 @@ func (e *tidbChecksumExecutor) Checksum(ctx context.Context, tableInfo *checkpoi
// +---------+------------+---------------------+-----------+-------------+

cs := RemoteChecksum{}
err = common.SQLWithRetry{DB: e.db, Logger: task.Logger}.QueryRow(ctx, "compute remote checksum",
exec := common.SQLWithRetry{DB: e.db, Logger: task.Logger}
if err := exec.Exec(ctx, "increase tidb_backoff_weight", fmt.Sprintf("SET SESSION tidb_backoff_weight = '%d';", 3*tikvstore.DefBackOffWeight)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make backoff weight a configurable parameter ?

disttask/importinto/subtask_executor.go Outdated
if !ok {
return errors.New("tidb_backoff_weight not set")
}
backoffWeight := mathutil.Max(backoffWeightBackup, strconv.Itoa(3*tikvstore.DefBackOffWeight))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make backoff weight a configurable parameter ?

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 20, 2023

/run-integration-br-test

1 similar comment
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 20, 2023

/run-integration-br-test

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 21, 2023

/run-integration-br-test

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 21, 2023

/run-integration-br-test

@GMHDBJD GMHDBJD mentioned this pull request Jun 21, 2023
Merged
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 21, 2023

/run-cherry-picker

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.2: #44884.

@ti-chi-bot ti-chi-bot mentioned this pull request Jun 21, 2023
Merged
12 tasks
@pingcap pingcap deleted a comment from tiprow bot Jun 21, 2023
ti-chi-bot bot pushed a commit that referenced this pull request Jun 21, 2023
@ti-chi-bot
importinto/lightning: do remote checksum via sql (#44803) (#44884)
63b88e9 
close #41941, ref #44711
@niubell niubell added needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. labels Jun 21, 2023
@ti-chi-bot ti-chi-bot mentioned this pull request Jun 21, 2023
Merged
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.1: #44886.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 21, 2023
@GMHDBJD @ti-chi-bot
This is an automated cherry-pick of pingcap#44803
d0bb20d 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #44887.

@ti-chi-bot ti-chi-bot mentioned this pull request Jun 21, 2023
Closed
12 tasks
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 21, 2023
@GMHDBJD @ti-chi-bot
This is an automated cherry-pick of pingcap#44803
d19162e 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
D3Hunter added a commit that referenced this pull request Jun 27, 2023
@D3Hunter
Revert "importinto/lightning: do remote checksum via sql (#44803) (#4…
f6275f8 
…4884)"

This reverts commit 63b88e9.
GMHDBJD added a commit to ti-chi-bot/tidb that referenced this pull request Jun 29, 2023
@GMHDBJD
Revert "This is an automated cherry-pick of pingcap#44803"
3fbc648 
This reverts commit d0bb20d.
GMHDBJD added a commit to ti-chi-bot/tidb that referenced this pull request Jun 29, 2023
@GMHDBJD
Revert "This is an automated cherry-pick of pingcap#44803"
7a2bdad 
This reverts commit d19162e.
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 29, 2023

/run-cherry-picker

@GMHDBJD GMHDBJD added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Jun 29, 2023
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Jun 29, 2023

/run-cherry-picker

@ti-chi-bot ti-chi-bot mentioned this pull request Jun 29, 2023
Merged
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.1: #45055.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 29, 2023
@GMHDBJD @ti-chi-bot
This is an automated cherry-pick of pingcap#44803
3a13785 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
GMHDBJD added a commit to ti-chi-bot/tidb that referenced this pull request Jun 29, 2023
@GMHDBJD
Revert "This is an automated cherry-pick of pingcap#44803"
0069c96 
This reverts commit 3a13785.
ti-chi-bot bot pushed a commit that referenced this pull request Jun 30, 2023
@ti-chi-bot
importinto/lightning: do remote checksum via sql (#44803) (#44886)
bcaeb20 
close #41941, ref #44711
@D3Hunter
Copy link
Contributor

D3Hunter commented Jul 6, 2023

/cherry-pick release-7.1-20230706-v7.1.0

@ti-chi-bot
Copy link
Member

@D3Hunter: new pull request created to branch release-7.1-20230706-v7.1.0: #45195.

In response to this:

/cherry-pick release-7.1-20230706-v7.1.0

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.

@ti-chi-bot ti-chi-bot mentioned this pull request Jul 6, 2023
Merged
12 tasks
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 6, 2023
@GMHDBJD @ti-chi-bot
This is an automated cherry-pick of pingcap#44803
a49d278 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot bot pushed a commit that referenced this pull request Jul 6, 2023
@ti-chi-bot
importinto/lightning: do remote checksum via sql (#44803) (#45195)
9709d8e 
close #41941, ref #44711
ti-chi-bot bot pushed a commit that referenced this pull request Jul 12, 2023
@ti-chi-bot
importinto/lightning: do remote checksum via sql (#44803) (#45055)
dd6b7c3 
close #41941, ref #44711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsdashun dsdashun dsdashun left review comments

@niubell niubell niubell left review comments

@lance6716 lance6716 lance6716 approved these changes

@D3Hunter D3Hunter D3Hunter approved these changes

Assignees
No one assigned
Labels
approved lgtm needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.2 ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lightning: remote checksum call SQL than function
6 participants
@GMHDBJD @ti-chi-bot @D3Hunter @dsdashun @lance6716 @niubell