lightning: improve post-import conflict detection 'error' semantic #51277

lyzx2001 · 2024-02-23T06:49:46Z

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?

Improve post-import conflict detection '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-02-23T06:50:01Z

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-02-23T07:06:52Z

Codecov Report

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #51277        +/-   ##
================================================
+ Coverage   72.4691%   73.5567%   +1.0876%     
================================================
  Files          1474       1475         +1     
  Lines        363628     437945     +74317     
================================================
+ Hits         263518     322138     +58620     
- Misses        80708      95538     +14830     
- Partials      19402      20269       +867

Flag	Coverage Δ
integration	`50.4922% <39.4285%> (?)`
unit	`70.4977% <25.1428%> (-1.8548%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (-2.3014%)`	⬇️
parser	`∅ <ø> (∅)`
br	`56.5143% <58.5585%> (+10.1013%)`	⬆️

lyzx2001 · 2024-02-27T04:45:05Z

/test pull-lightning-integration-test

tiprow · 2024-02-27T04:45:26Z

@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-27T05:00:52Z

/test pull-lightning-integration-test

tiprow · 2024-02-27T05:01:13Z

@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-27T05:44:49Z

/test pull-lightning-integration-test

tiprow · 2024-02-27T05:45:10Z

@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-27T05:54:03Z

/test pull-lightning-integration-test

tiprow · 2024-02-27T05:54:27Z

@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-27T06:45:24Z

/test pull-lightning-integration-test

tiprow · 2024-02-27T06:45:47Z

@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-03-12T09:38:31Z

/test pull-lightning-integration-test

tiprow · 2024-03-12T09:38:53Z

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

D3Hunter

25/30

D3Hunter · 2024-03-12T09:56:52Z

pkg/table/tables/index_test.go

+
+	valueStr, err := tables.GenIndexValueFromIndex(indexKey, indexValue, tbl.Meta(), idxInfo)
+	require.NoError(t, err)
+	require.Equal(t, []string{"23"}, valueStr)


what's the output for binary or bits?

tables.GenIndexValueFromIndex's content is retrieved from the original function genKeyExistsErr in ddl

tidb/pkg/ddl/index.go

Lines 1601 to 1629 in 5745d3d

func genKeyExistsErr(key, value []byte, idxInfo *model.IndexInfo, tblInfo *model.TableInfo) error {

idxColLen := len(idxInfo.Columns)

indexName := fmt.Sprintf("%s.%s", tblInfo.Name.String(), idxInfo.Name.String())

colInfos := tables.BuildRowcodecColInfoForIndexColumns(idxInfo, tblInfo)

values, err := tablecodec.DecodeIndexKV(key, value, idxColLen, tablecodec.HandleNotNeeded, colInfos)

if err != nil {

logutil.BgLogger().Warn("decode index key value failed", zap.String("index", indexName),

zap.String("key", hex.EncodeToString(key)), zap.String("value", hex.EncodeToString(value)), zap.Error(err))

return kv.ErrKeyExists.FastGenByArgs(key, indexName)

}

valueStr := make([]string, 0, idxColLen)

for i, val := range values[:idxColLen] {

d, err := tablecodec.DecodeColumnValue(val, colInfos[i].Ft, time.Local)

if err != nil {

logutil.BgLogger().Warn("decode column value failed", zap.String("index", indexName),

zap.String("key", hex.EncodeToString(key)), zap.String("value", hex.EncodeToString(value)), zap.Error(err))

return kv.ErrKeyExists.FastGenByArgs(key, indexName)

}

str, err := d.ToString()

if err != nil {

str = string(val)

}

if types.IsBinaryStr(colInfos[i].Ft) || types.IsTypeBit(colInfos[i].Ft) {

str = util.FmtNonASCIIPrintableCharToHex(str)

}

valueStr = append(valueStr, str)

}

return kv.ErrKeyExists.FastGenByArgs(strings.Join(valueStr, "-"), indexName)

}

zimulala · 2024-03-13T03:07:41Z

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

+	require.EqualError(t, newErr, "[Lightning:Restore:ErrFoundIndexConflictRecords]found index conflict records in table a, index name is 'a.key_b', unique key is '[7]', primary key is '3'")
+}
+
+func TestConvertToErrFoundConflictRecordsMultipleColumnsIndex(t *testing.T) {


TestConvertToErrFoundConflictRecords and TestConvertToErrFoundConflictRecordsMultipleColumnsIndex only difference seems to be the number of columns in the table and the index. Could we unify it?

The three unit tests have been unified by extracting buildTableForTestConvertToErrFoundConflictRecords function.

lyzx2001 · 2024-03-13T04:55:35Z

/test pull-lightning-integration-test

tiprow · 2024-03-13T04:55:56Z

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

D3Hunter

rest lgtm

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

D3Hunter · 2024-03-13T04:47:33Z

br/pkg/lightning/common/errors.go

+	ErrInvalidMetaStatus         = errors.Normalize("invalid meta status: '%s'", errors.RFCCodeText("Lightning:Restore:ErrInvalidMetaStatus"))
+	ErrTableIsChecksuming        = errors.Normalize("table '%s' is checksuming", errors.RFCCodeText("Lightning:Restore:ErrTableIsChecksuming"))
+	ErrResolveDuplicateRows      = errors.Normalize("resolve duplicate rows error on table '%s'", errors.RFCCodeText("Lightning:Restore:ErrResolveDuplicateRows"))
+	ErrFoundDuplicateKeys        = errors.Normalize("found duplicate key '%s', value '%s'", errors.RFCCodeText("Lightning:Restore:ErrFoundDuplicateKey"))


maybe add a todo here, this error should be delete and replace with below 2 more concrete error

D3Hunter · 2024-03-13T04:54:20Z

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

-		return false, errors.Trace(err)
+	err = duplicateManager.CollectDuplicateRowsFromTiKV(ctx, local.importClientFactory, algorithm)
+	if err != nil {
+		return common.ErrFoundDataConflictRecords.Equal(err) || common.ErrFoundIndexConflictRecords.Equal(err), errors.Trace(err)


hasDup should be useless if error happened, just return false?

and seems CollectDuplicateRowsFromTiKV might return ErrFoundDuplicateKeys

see below comment

I don' think hasDup is useless. If error happened, it may be other error like decoding failed or other failure, not necessarily means we encounter duplicate. We still need common.ErrFoundDataConflictRecords.Equal(err) || common.ErrFoundIndexConflictRecords.Equal(err) to determine whether we have duplicates.

CollectDuplicateRowsFromTiKV will not return ErrFoundDuplicateKeys. Here it can only return ErrFoundDataConflictRecords or ErrFoundIndexConflictRecords.

D3Hunter · 2024-03-13T04:58:59Z

pkg/ddl/ingest/backend.go

@@ -90,6 +92,33 @@ type litBackendCtx struct {
 	etcdClient      *clientv3.Client
 }

+func (bc *litBackendCtx) handleErrorAfterCollectRemoteDuplicateRows(err error, indexID int64, tbl table.Table, hasDupe bool) error {
+	if err != nil && !common.ErrFoundIndexConflictRecords.Equal(err) {


if there's error, using hasDupe is not a common style, especially we are passing some context message through the error, it's not a good way to do it. i prefer we return 2 values (*DupInfo, error), error indicate something that we cannot handle any further, DupInfo is not null when there's duplicates found and it hold it's context information

I think here the error is correctly handled. We first determine
if err != nil && !common.ErrFoundIndexConflictRecords.Equal(err) .
In this case, the error means something that we cannot handle any further.

Then we use hasDupe, in this case either we have no error, or the error is ErrFoundIndexConflictRecords. In both cases, we can directly use hasDupe for further operation.

it's very uncommon to used other return values when there's error returned

and this error is actually works as a struct to pass context info when duplicate is detected. so i prefer write in the way in previous comment

if there's no time to change, i think we can leave a todo to do it later

lyzx2001 · 2024-03-13T07:10:41Z

/test pull-lightning-integration-test

tiprow · 2024-03-13T07:11:02Z

@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-03-13T08:43:00Z

/test pull-lightning-integration-test

tiprow · 2024-03-13T08:43:21Z

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

ti-chi-bot · 2024-03-13T09:07:51Z

[LGTM Timeline notifier]

Timeline:

2024-03-12 02:47:13.298313837 +0000 UTC m=+740060.320560226: ☑️ agreed by Leavrth.
2024-03-13 09:07:50.835466181 +0000 UTC m=+849297.857712554: ☑️ agreed by zimulala.

lyzx2001 · 2024-03-13T09:37:21Z

/test pull-lightning-integration-test

tiprow · 2024-03-13T09:37: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.

D3Hunter · 2024-03-13T09:44:54Z

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

 		return false, errors.Trace(err)
 	}
 	return duplicateManager.HasDuplicate(), nil
 }

 // CollectRemoteDuplicateRows collect duplicate keys from remote TiKV storage. This keys may be duplicate with
 // the data import by other lightning.
-func (local *DupeController) CollectRemoteDuplicateRows(ctx context.Context, tbl table.Table, tableName string, opts *encode.SessionOptions) (hasDupe bool, err error) {
+// TODO: revise the returned arguments to (hasDupe bool, dupInfo *DupInfo, err error) to distinguish the conflict error and the common error


Suggested change

// TODO: revise the returned arguments to (hasDupe bool, dupInfo *DupInfo, err error) to distinguish the conflict error and the common error

// TODO: revise the returned arguments to (dupInfo *DupInfo, err error) to distinguish the conflict error and the common error

hasDup can be infered from dupInfo != nil

Maybe not necessarily? Only when conflict detection mode is set to error, CollectDuplicateRowsFromTiKV would return ErrFoundDataConflictRecords or ErrFoundIndexConflictRecords. For other modes, we still need duplicateManager.HasDuplicate() to determine whether we have duplicate. And this information is needed for lightning

tidb/br/pkg/lightning/importer/table_import.go

Lines 1055 to 1059 in 7548df7

hasRemoteDupe, e := dupeController.CollectRemoteDuplicateRows(ctx, tr.encTable, tr.tableName, opts)

if e != nil {

tr.logger.Error("collect remote duplicate keys failed", log.ShortError(e))

return false, errors.Trace(e)

}

ti-chi-bot · 2024-03-13T09:45:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [D3Hunter,Leavrth,zimulala]
~~br/OWNERS~~ [Leavrth]
~~br/pkg/lightning/OWNERS~~ [D3Hunter]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/table/OWNERS~~ [zimulala]

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

lightning: improve post-import conflict detection 'error' semantic

046572b

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 23, 2024

lyzx2001 changed the title ~~[WIP] lightning: improve post-import conflict detection 'error' semantic~~ lightning: improve post-import conflict detection 'error' semantic Feb 27, 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 27, 2024

add function ConvertToErrFoundConflictRecords

32dd4cb

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 27, 2024

Merge branch 'master' into merge_conflict_detection_2

ec0fa54

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

update ut

3b3fc76

update table_import.go

fcbadf6

update table_import.go

5046519

update

d4ea7af

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 27, 2024

lyzx2001 requested review from zimulala and D3Hunter March 12, 2024 09:38

D3Hunter reviewed Mar 12, 2024

View reviewed changes

zimulala reviewed Mar 13, 2024

View reviewed changes

extract buildTableForTestConvertToErrFoundConflictRecords

856fbd4

lyzx2001 requested review from zimulala and D3Hunter March 13, 2024 04:56

D3Hunter reviewed Mar 13, 2024

View reviewed changes

merge master

4827b25

update according to comments

daee5e4

lyzx2001 requested a review from D3Hunter March 13, 2024 08:43

zimulala approved these changes Mar 13, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels Mar 13, 2024

update according to comments

f8021a2

D3Hunter approved these changes Mar 13, 2024

View reviewed changes

ti-chi-bot bot added the approved label Mar 13, 2024

ti-chi-bot bot merged commit ccc453b into pingcap:master Mar 13, 2024
26 checks passed

	func genKeyExistsErr(key, value []byte, idxInfo model.IndexInfo, tblInfo model.TableInfo) error {
	idxColLen := len(idxInfo.Columns)
	indexName := fmt.Sprintf("%s.%s", tblInfo.Name.String(), idxInfo.Name.String())
	colInfos := tables.BuildRowcodecColInfoForIndexColumns(idxInfo, tblInfo)
	values, err := tablecodec.DecodeIndexKV(key, value, idxColLen, tablecodec.HandleNotNeeded, colInfos)
	if err != nil {
	logutil.BgLogger().Warn("decode index key value failed", zap.String("index", indexName),
	zap.String("key", hex.EncodeToString(key)), zap.String("value", hex.EncodeToString(value)), zap.Error(err))
	return kv.ErrKeyExists.FastGenByArgs(key, indexName)
	}
	valueStr := make([]string, 0, idxColLen)
	for i, val := range values[:idxColLen] {
	d, err := tablecodec.DecodeColumnValue(val, colInfos[i].Ft, time.Local)
	if err != nil {
	logutil.BgLogger().Warn("decode column value failed", zap.String("index", indexName),
	zap.String("key", hex.EncodeToString(key)), zap.String("value", hex.EncodeToString(value)), zap.Error(err))
	return kv.ErrKeyExists.FastGenByArgs(key, indexName)
	}
	str, err := d.ToString()
	if err != nil {
	str = string(val)
	}
	if types.IsBinaryStr(colInfos[i].Ft) \|\| types.IsTypeBit(colInfos[i].Ft) {
	str = util.FmtNonASCIIPrintableCharToHex(str)
	}
	valueStr = append(valueStr, str)
	}
	return kv.ErrKeyExists.FastGenByArgs(strings.Join(valueStr, "-"), indexName)
	}

	// TODO: revise the returned arguments to (hasDupe bool, dupInfo *DupInfo, err error) to distinguish the conflict error and the common error
	// TODO: revise the returned arguments to (dupInfo *DupInfo, err error) to distinguish the conflict error and the common error

	hasRemoteDupe, e := dupeController.CollectRemoteDuplicateRows(ctx, tr.encTable, tr.tableName, opts)
	if e != nil {
	tr.logger.Error("collect remote duplicate keys failed", log.ShortError(e))
	return false, errors.Trace(e)
	}

lightning: improve post-import conflict detection 'error' semantic #51277

lightning: improve post-import conflict detection 'error' semantic #51277

Conversation

lyzx2001 commented Feb 23, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Feb 23, 2024

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

lyzx2001 commented Feb 27, 2024

tiprow bot commented Feb 27, 2024

lyzx2001 commented Feb 27, 2024

tiprow bot commented Feb 27, 2024

lyzx2001 commented Feb 27, 2024

tiprow bot commented Feb 27, 2024

lyzx2001 commented Feb 27, 2024

tiprow bot commented Feb 27, 2024

lyzx2001 commented Feb 27, 2024

tiprow bot commented Feb 27, 2024

lyzx2001 commented Mar 12, 2024

tiprow bot commented Mar 12, 2024

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

lyzx2001 commented Mar 13, 2024

tiprow bot commented Mar 13, 2024

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

D3Hunter Mar 13, 2024 • edited Loading

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 Mar 13, 2024

tiprow bot commented Mar 13, 2024

lyzx2001 commented Mar 13, 2024

tiprow bot commented Mar 13, 2024

ti-chi-bot bot commented Mar 13, 2024

[LGTM Timeline notifier]

lyzx2001 commented Mar 13, 2024

tiprow bot commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Mar 13, 2024

codecov bot commented Feb 23, 2024 •

edited

Loading

D3Hunter Mar 13, 2024 •

edited

Loading