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

kvclient: filter the old value if old-value is disabled #1304

Merged
merged 25 commits into from
Jan 26, 2021
Merged

kvclient: filter the old value if old-value is disabled #1304

merged 25 commits into from
Jan 26, 2021

Conversation

zier-one
Copy link
Contributor

What problem does this PR solve?

fix #1299

What is changed and how it works?

set Oldvalue to nil when old-value is disabled

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix the old-value switch of a changefeed could be affected by another changefeed

@zier-one zier-one added the status/ptal Could you please take a look? label Jan 14, 2021
@zier-one
Copy link
Contributor Author

/run-all-tests

amyangfei
amyangfei reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

Recommend adding an integration test

amyangfei
amyangfei reviewed Jan 14, 2021
View reviewed changes
cdc/kv/matcher.go Show resolved Hide resolved
cdc/kv/client.go Outdated Show resolved Hide resolved
cdc/kv/matcher.go Show resolved Hide resolved
cdc/kv/client.go Show resolved Hide resolved
overvenus
overvenus reviewed Jan 15, 2021
View reviewed changes
errors.toml Outdated Show resolved Hide resolved
amyangfei
amyangfei reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 15, 2021
@overvenus
Copy link
Member

Could you add some old value integration tests?

@amyangfei amyangfei added the needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. label Jan 19, 2021
@zier-one
Copy link
Contributor Author

Could you add some old value integration tests?

what kind of old-value tests?

@overvenus
Copy link
Member

An end to end test, multiple changefeeds with different old-value setting, we need make sure they do not interfere each other.

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

zier-one
zier-one commented Jan 22, 2021
View reviewed changes
tests/multi_changefeed/run.sh Outdated Show resolved Hide resolved
tests/multi_changefeed/run.sh Outdated Show resolved Hide resolved
@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one
Copy link
Contributor Author

/run-integration-tests

@liuzix
Copy link
Contributor

liuzix commented Jan 26, 2021

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 26, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 26, 2021
@liuzix
Copy link
Contributor

liuzix commented Jan 26, 2021

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 1335
  • 1317

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 26, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

1 similar comment
@amyangfei
Copy link
Contributor

/run-all-tests

@amyangfei
Copy link
Contributor

/run-check-release-note

1 similar comment
@amyangfei
Copy link
Contributor

/run-check-release-note

@ti-srebot ti-srebot merged commit 6a7d5ce into pingcap:master Jan 26, 2021
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Jan 26, 2021
@ti-srebot
cherry pick pingcap#1304 to release-4.0
3a2a57c 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 26, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1347

ti-srebot added a commit that referenced this pull request Jan 26, 2021
@ti-srebot
kvclient: filter the old value if old-value is disabled (#1304) (#1347)
eb66cf3 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei left review comments

@overvenus overvenus overvenus left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

old value feature can't be turned off if changefeed has enabled it
5 participants
@zier-one @overvenus @liuzix @ti-srebot @amyangfei