lightning: remove post-import conflict detection "remove" and "record" semantic, and add "error" semantic #50806

lyzx2001 · 2024-01-30T04:29:36Z

What problem does this PR solve?

Issue Number: ref #51036

Problem Summary:
Merge preprocess duplicate detection and post-import conflict detection.

What changed and how does it work?

Remove post-import conflict detection "remove" and "record" semantic, and add "error" semantic.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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

tiprow · 2024-01-30T04:29:53Z

Hi @lyzx2001. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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/test-infra repository.

codecov · 2024-01-30T04:43:30Z

Codecov Report

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #50806        +/-   ##
================================================
+ Coverage   70.4926%   73.8217%   +3.3291%     
================================================
  Files          1463       1469         +6     
  Lines        433262     453671     +20409     
================================================
+ Hits         305418     334908     +29490     
+ Misses       108580      98093     -10487     
- Partials      19264      20670      +1406

Flag	Coverage Δ
integration	`50.9058% <27.3809%> (?)`
unit	`70.3137% <51.1904%> (+0.0289%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.2940% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`54.9448% <55.9523%> (+9.2085%)`	⬆️

…mantic

lyzx2001 · 2024-02-07T09:31:13Z

/test pull-lightning-integration-test

tiprow · 2024-02-07T09:31:35Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-07T10:26:22Z

/test pull-lightning-integration-test

tiprow · 2024-02-07T10:26:46Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-07T11:40:27Z

/test pull-lightning-integration-test

tiprow · 2024-02-07T11:40:49Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-07T12:57:16Z

/test pull-lightning-integration-test

tiprow · 2024-02-07T12:57:39Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-07T13:34:44Z

/test pull-lightning-integration-test

tiprow · 2024-02-07T13:35:05Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lance6716

/hold

need to address comment

ti-chi-bot · 2024-02-18T09:10:19Z

[LGTM Timeline notifier]

Timeline:

2024-02-18 08:49:40.342214893 +0000 UTC m=+174269.089837996: ☑️ agreed by D3Hunter.
2024-02-18 09:10:18.365572226 +0000 UTC m=+175507.113195337: ☑️ agreed by lance6716.

lyzx2001 · 2024-02-18T10:17:27Z

/test pull-lightning-integration-test

tiprow · 2024-02-18T10:17:48Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-18T12:30:13Z

/test pull-lightning-integration-test

tiprow · 2024-02-18T12:30:34Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-18T12:52:43Z

/test pull-lightning-integration-test

tiprow · 2024-02-18T12:53:03Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-18T14:23:29Z

/test pull-lightning-integration-test

tiprow · 2024-02-18T14:23:51Z

@lyzx2001: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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 kubernetes/test-infra repository.

lyzx2001 · 2024-02-19T03:49:27Z

/cc @3pointer

lance6716

rest lgtm

lance6716 · 2024-02-19T03:41:01Z

br/pkg/lightning/errormanager/errormanager.go

+	_, gCtx := errgroup.WithContext(ctx)
+
+	kvRows, err := em.db.QueryContext(
+		gCtx, common.SprintfWithIdentifiers(selectConflictKeysCountError, em.schema),


no need to query the count(*). we can query all data and check if result is empty

lance6716 · 2024-02-19T03:44:46Z

br/pkg/lightning/errormanager/errormanager.go

+// ResolveConflictKeysError query all conflicting rows (handle and their
+// values) from the current error report and return error
+// if the number of the conflicting rows is larger than 0.
+func (em *ErrorManager) ResolveConflictKeysError(


it's better we implement the "error" setting in dupeController.CollectRemoteDuplicateRows. That function will fail earlier when see the first error. In this location we will record all errors which may cost a lot.

You can open another PR to do it.

OK. Will implement in the next PR.

lance6716 · 2024-02-19T03:51:27Z

br/tests/lightning_duplicate_resolution_error/run.sh

+! run_lightning --backend local --config "${mydir}/config.toml"
+[ $? -eq 0 ]
+
+tail -n 10 $TEST_DIR/lightning.log | grep "ERROR" | tail -n 1 | grep -Fq "[Lightning:Restore:ErrFoundDuplicateKey]found duplicate key"


can you also check the output key is human readable? also ask PM what's the expected format.

Here I conform to the output for the error mode of preprocess detection

tidb/br/pkg/lightning/common/dupdetect.go

Lines 96 to 102 in a465300

if d.option.ReportErrOnDup {

dupKey := make([]byte, len(d.curKey))

dupVal := make([]byte, len(val))

copy(dupKey, d.curKey)

copy(dupVal, d.curVal)

return nil, nil, false, ErrFoundDuplicateKeys.FastGenByArgs(dupKey, dupVal)

}

I think users want unified output no matter the conflicts come from preprocess detection or post-import detection.
Let me check with PM for this.

Currently the output looks like this

[error="[Lightning:Restore:ErrFoundDuplicateKey]found duplicate key 't\ufffd\u0000\u0000\u0000\u0000\u0000\u0000l_r\ufffd\u0000\u0000\u0000\u0000\u0000\u0000\u0001', value '\ufffd\u0000\u0002\u0000\u0000\u0000\u0002\u0003\u0001\u0000\u0006\u0000\u00061.csv'"]

"error" is only internally used before, it does not consider human-readability.

DDL uses "error" option before, and has extra work to extract the data, like

if common.ErrFoundDuplicateKeys.Equal(err) { err = convertToKeyExistsErr(err, indexInfo, tbl.Meta()) }

We can mimic it or we directly read other fields from the conflict error table and display them to user. Please be careful do not break DDL's behaviour.

Will address in next PR.

lyzx2001 · 2024-02-19T04:13:07Z

/unhold

lyzx2001 · 2024-02-19T04:45:57Z

/cc @Leavrth

ti-chi-bot · 2024-02-19T04:50:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, lance6716, Leavrth

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [Leavrth]
~~br/pkg/lightning/OWNERS~~ [D3Hunter,lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

test Preprocess Duplicate Detection and Post-Import Conflict Detection

adf4eff

lyzx2001 changed the title ~~[WIP] lightning: merge preprocess duplicate detection and post-import conflict detection~~ lightning: remove post-import conflict detection "remove" and "record" semantic Feb 7, 2024

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

lightning: remove post-import conflict detection remove and record se…

a090b9d

…mantic

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/needs-linked-issue labels Feb 7, 2024

update

94b397c

update

4672e69

update

a6a58b6

update

62ccd50

update

3f94344

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2024

lance6716 approved these changes Feb 18, 2024

View reviewed changes

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 18, 2024

lyzx2001 added 2 commits February 18, 2024 17:18

modify error test that duplicate data are saved in different files

426d710

modify error test

01b40cf

modify error test for conflicts across engines

e8854d5

modify error test for conflicts across engines

0dadb83

modify error test for conflicts across engines

df67d8d

ti-chi-bot bot requested a review from 3pointer February 19, 2024 03:49

lance6716 reviewed Feb 19, 2024

View reviewed changes

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

ti-chi-bot bot requested a review from Leavrth February 19, 2024 04:45

Leavrth approved these changes Feb 19, 2024

View reviewed changes

ti-chi-bot bot added the approved label Feb 19, 2024

ti-chi-bot bot merged commit e3e0f7e into pingcap:master Feb 19, 2024
26 checks passed

lyzx2001 mentioned this pull request Feb 26, 2024

lightning failed with error “tidb lightning encountered error: invalid duplicate-resolution 'record', please choose valid option between ['none', 'replace', 'error']” #51295

Closed

	if d.option.ReportErrOnDup {
	dupKey := make([]byte, len(d.curKey))
	dupVal := make([]byte, len(val))
	copy(dupKey, d.curKey)
	copy(dupVal, d.curVal)
	return nil, nil, false, ErrFoundDuplicateKeys.FastGenByArgs(dupKey, dupVal)
	}

lightning: remove post-import conflict detection "remove" and "record" semantic, and add "error" semantic #50806

lightning: remove post-import conflict detection "remove" and "record" semantic, and add "error" semantic #50806

Conversation

lyzx2001 commented Jan 30, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jan 30, 2024

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

lyzx2001 commented Feb 7, 2024

tiprow bot commented Feb 7, 2024

lyzx2001 commented Feb 7, 2024

tiprow bot commented Feb 7, 2024

lyzx2001 commented Feb 7, 2024

tiprow bot commented Feb 7, 2024

lyzx2001 commented Feb 7, 2024

tiprow bot commented Feb 7, 2024

lyzx2001 commented Feb 7, 2024

tiprow bot commented Feb 7, 2024

lance6716 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 18, 2024

[LGTM Timeline notifier]

lyzx2001 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

lyzx2001 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

lyzx2001 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

lyzx2001 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

lyzx2001 commented Feb 19, 2024

lance6716 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzx2001 commented Feb 19, 2024

lyzx2001 commented Feb 19, 2024

ti-chi-bot bot commented Feb 19, 2024

lyzx2001 commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading