lightning: refine fetching table structure error handling

[dsdashun]

What problem does this PR solve?

Issue Number: close #44614

Problem Summary:

What is changed and how it works?

If any errors are encountered while fetching column auto information, log a warning and refrain from adding the table structure to the table structure map. This approach allows the error to be ignored if the table is not relevant, making the importing process proceed without any issues.

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

[dsdashun]

/run-integration-br-test

[dsdashun]

/run-integration-br-test

[lance6716]

we can use nil inject function when the inject verb is sleep, like FailAfterWriteRows

[dsdashun]

Yes, I understand your point. I print that out to verify in the integration test script that my drop table is executed before this failpoint's hook is triggered. This makes it easier to debug. Do you think it's OK to keep the printing message here?

[lance6716]

Can the caller directly set FetchRemoteTableModels_BeforeFetchTableAutoIDInfos?

[dsdashun]

I have previously attempted this approach where I set a sleep timer of 5 seconds to activate that failpoint. During the integration test, the failpoint was triggered twice, resulting in a total of 10 seconds of delay. However, I was only concerned with the second trigger. In this second trigger, I needed to complete the drop table operation within the sleep period. To achieve this, I added an 8-second sleep timer in the integration test script (5 < 8 < 10). Although this approach worked, it lacked flexibility. If there were more or fewer calls to FetchRemoteTableModels, I would need to adjust the sleep timer accordingly.

Therefore, I now use a different approach where I introduce a failpoint here that enables the specific failpoint I want to sleep. This way, I only need to adjust the sleep time together with the total sleep time of a single failpoint trigger, making the behavior more controllable.

[lance6716]

I'm not sure this approach, are we currently equivalently set sleep(6000) for FetchRemoteTableModels_BeforeFetchTableAutoIDInfos? so we can directly set it.

And another suggestion is maybe we let FetchRemoteTableModels_BeforeFetchTableAutoIDInfos use b.db to drop the table, to mimic the user action happens at that point 😂 so it's no longer related to sleep duration

[dsdashun]

I'm not sure this approach, are we currently equivalently set sleep(6000) for FetchRemoteTableModels_BeforeFetchTableAutoIDInfos? so we can directly set it.

If directly enable FetchRemoteTableModels_BeforeFetchTableAutoIDInfos with sleep(6000), it will actually sleep 12 seconds. The first 6 seconds is from here:

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

Line 807 in e341791

err := worker.makeJobs(rc.dbMetas, rc.preInfoGetter.FetchRemoteTableModels)

=>

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

Line 601 in e341791

tables, _ := getTables(worker.ctx, dbMeta.Name)

At this time, the table schemas have not been created, and the errors are totally ignored.

The second 6 seconds is from here:

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

Line 813 in e341791

dbInfos, err := rc.preInfoGetter.GetAllTableStructures(ctx)

=>

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

Lines 356 to 358 in e341791

	dbInfos, err = LoadSchemaInfo(ctx, p.dbMetas, func(ctx context.Context, dbName string) ([]*model.TableInfo, error) {
	return p.getTableStructuresByFileMeta(ctx, p.mdDBMetaMap[dbName], getPreInfoCfg)
	})

=>

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

Line 368 in e341791

currentTableInfosFromDB, err := p.targetInfoGetter.FetchRemoteTableModels(ctx, dbName)

At this time, the table schemas have been created.

For my approach, I initially did not enable the FetchRemoteTableModels_BeforeFetchTableAutoIDInfos failpoint. After creating the schemas, I enabled the failpoint here to ensure that the tables have been created during the sleep time of the failpoint. This way, any errors will not be completely ignored.

For the alternative method you mentioned, we still need a precise way to trigger the failpoint only once during the second attempt, ensuring that the error will not be ignored. If the FetchRemoteTableModels_BeforeFetchTableAutoIDInfos failpoint is triggered twice and the drop table is executed twice, the following will occur:

• The first time, the drop table is executed successfully, but the generated error is ignored.
• The second time, the drop table cannot be executed successfully because the table has already been dropped. We won't encounter any error because the drop table is not executed halfway.

[dsdashun]

And another suggestion is maybe we let FetchRemoteTableModels_BeforeFetchTableAutoIDInfos use b.db to drop the table, to mimic the user action happens at that point 😂 so it's no longer related to sleep duration

This suggestion is technically feasible and presents a good option. However, I am currently struggling with whether to include "DROP TABLE" in the script or in the failpoint hook.

• If we write "DROP TABLE" in the script, it offers flexibility as only the script needs to be modified. However, this approach also has some drawbacks, such as unnecessary additional sleep time that may cause uncontrollable execution sequences. We would then need to tune the sleep time in the script.
• On the other hand, if we write "DROP TABLE" in the failpoint, it eliminates the need for additional sleep time and provides controllable logic. However, it may feel like we are coupling the testing logic into the Lightning code.

Which approach do you prefer?

[lance6716]

currently I prefer we use failpoint to do it. Let's wait the option from the second reviewer

[lance6716]

In the lightning logic, is it easy to pass the table-level allow list to this function, so we don't need to care about unrelated tables?

No need to change in this PR

[lance6716]

Can you check if this idea will work? But we don't need to change this PR

[lance6716]

currently I prefer we use failpoint to do it. Let's wait the option from the second reviewer

[lance6716]

Can you check if this idea will work? But we don't need to change this PR

[GMHDBJD]

LGTM

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GMHDBJD, lance6716

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 [GMHDBJD,lance6716]

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

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2023-06-26 02:53:07.307042309 +0000 UTC m=+589752.708292752: ☑️ agreed by lance6716.
• 2023-06-29 09:04:12.231916705 +0000 UTC m=+871217.633167139: ☑️ agreed by GMHDBJD.

[dsdashun]

/retest-required

[tiprow]

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

In response to this:

/retest-required

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.

[dsdashun]

/run-integration-br-test

[ti-chi-bot]

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

[ti-chi-bot]

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

[ti-chi-bot]

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

[ti-chi-bot]

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

[ti-chi-bot]

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

[ti-chi-bot]

@dsdashun: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/check_dev_2	9c6259e	link	unknown	/test check-dev2

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[ti-chi-bot]

@dsdashun: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/check_dev_2	9c6259e	link	unknown	/test check-dev2

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. I understand the commands that are listed here.

[dsdashun]

/cherry-pick release-6.5-20230424-v6.5.2

[ti-chi-bot]

@dsdashun: new pull request created to branch release-6.5-20230424-v6.5.2: #45082.

In response to this:

/cherry-pick release-6.5-20230424-v6.5.2

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.