owner(ticdc): Fix an unexpected stop problem for changefeed backoff

[zhaoxinyu]

What problem does this PR solve?

Issue Number: close #6348

What is changed and how it works?

Use 0 as MaxElapsedTime value instead of not populating this field. Because if MaxElapsedTime is not set,
15 min will be used in default. So after 15min, the backoff will stop, which is unexpected.
Ref https://github.com/cenkalti/backoff/blob/6b0e4ad0cd65431b217393bff47d1dff727b264b/exponential.go#L80

Check List

Tests

• Unit test
• Integration test

Questions

Will it cause performance regression or break compatibility?

No

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

No

Release note

None

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• asddongmen
• overvenus

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.

[zhaoxinyu]

/cc @asddongmen
/cc @overvenus
/cc @sdojjy

[amyangfei]

Note the affected versions should be updated, since #4262 was picked to all release branches. @zhaoxinyu

[overvenus]

Please add unit tests. Also make sure when a processor reports an error, owner close its changefeed immediately.

[zhaoxinyu]

Please add unit tests. Also make sure when a processor reports an error, owner close its changefeed immediately.

@overvenus We already have a test case TestHandleError to cover the backoff retry. The root cause of this issue is due to the hidden DefaultMaxElapsedTime = 15 * time.Minute in the backoff lib. I thought about adding more intervals in the following lines to let the total elapsed time pass 15min:

tiflow/cdc/owner/feed_state_manager_test.go

Lines 267 to 272 in f86dd06

	intervals := []time.Duration{200, 400, 800, 1600, 1600}
	for i, d := range intervals {
	intervals[i] = d * time.Millisecond
	}
	for _, d := range intervals {

However, 15min is extremely long for a unit test. Do you have any suggestions?

[zhaoxinyu]

Note the affected versions should be updated, since #4262 was picked to all release branches. @zhaoxinyu

The problem fixed by this PR is introduced in #6020, which was in a feature branch. In that PR, defaultBackoffMaxElapsedTime was removed to solve another issue #5647. It should be solved in master branch instead of in the feature branch. It is my mistake.

Maybe I should submit an other standalone PR in 6.1 branch and cherry-pick it back? @amyangfei How do you think?

[overvenus]

Please add unit tests. Also make sure when a processor reports an error, owner close its changefeed immediately.

@overvenus We already have a test case TestHandleError to cover the backoff retry. The root cause of this issue is due to the hidden DefaultMaxElapsedTime = 15 * time.Minute in the backoff lib. I thought about adding more intervals in the following lines to let the total elapsed time pass 15min:

tiflow/cdc/owner/feed_state_manager_test.go

Lines 267 to 272 in f86dd06

	intervals := []time.Duration{200, 400, 800, 1600, 1600}
	for i, d := range intervals {
	intervals[i] = d * time.Millisecond
	}
	for _, d := range intervals {

However, 15min is extremely long for a unit test. Do you have any suggestions?

What about setting a smaller backoff interval in tests?

[zhaoxinyu]

What about setting a smaller backoff interval in tests?

@overvenus , in the unit tests, I think the backoff interval is already small enough. Ref:

tiflow/cdc/owner/feed_state_manager.go

Lines 78 to 84 in 899d50e

	func newFeedStateManager4Test() *feedStateManager {
	f := new(feedStateManager)
	f.errBackoff = backoff.NewExponentialBackOff()
	f.errBackoff.InitialInterval = 200 * time.Millisecond
	f.errBackoff.MaxInterval = 1600 * time.Millisecond
	f.errBackoff.Multiplier = 2.0

[overvenus]

so make sure when a processor reports an error, owner close its changefeed immediately.

Okay, we need a test as mentioned above.

[amyangfei]

Note the affected versions should be updated, since #4262 was picked to all release branches. @zhaoxinyu

The problem fixed by this PR is introduced in #6020, which was in a feature branch. In that PR, defaultBackoffMaxElapsedTime was removed to solve another issue #5647. It should be solved in master branch instead of in the feature branch. It is my mistake.

Maybe I should submit an other standalone PR in 6.1 branch and cherry-pick it back? @amyangfei How do you think?

looks fine

[asddongmen]

/lgtm

[asddongmen]

/merge

[ti-chi-bot]

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

Commit hash: 6a699ae

[asddongmen]

/merge

[ti-chi-bot]

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

Commit hash: 1b50c47

[zhaoxinyu]

/run-dm-compatibility-test
/run-dm-integration-test
/run-integration-test
/run-kafka-integration-test

[zhaoxinyu]

/run-integration-test
/run-kafka-integration-test

[zhaoxinyu]

/run-all-tests

[zhaoxinyu]

/run-verify

[zhaoxinyu]

/run-kafka-integration-test

[zhaoxinyu]

/run-integration-test

[ti-chi-bot]

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

[zhaoxinyu]

/run-integration-test

[ti-chi-bot]

In response to a cherrypick label: cannot checkout release-6.2: error checking out release-6.2: exit status 1. output: error: pathspec 'release-6.2' did not match any file(s) known to git