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

*: chose one index to output delete events #787

Merged
merged 25 commits into from
Aug 3, 2020
Merged

*: chose one index to output delete events #787

merged 25 commits into from
Aug 3, 2020

Conversation

zier-one
Copy link
Contributor

@zier-one zier-one commented Jul 22, 2020 •
edited
Loading

What problem does this PR solve?

fix the #728

What is changed and how it works?

  • we will select one unique index(including primary key) as the handle index for every table
  • the delete events are generated based on the handle index
  • the column of handle index will be marked in column flag
  • if a unique index includes a virtual generated column, we regard the index as invalid.
  • the stored generated columns are captured now and it will be also marked in column flag
  • provides a new dispatcher named index-value
  • the rwoid dispatcher is instead by index-value
  • the where_handle field in Column is deprecation

Check List

Tests

  • Unit test
  • Integration test

Release note

  • select one unique index as the handle index to generate the delete events

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one zier-one added the status/ptal Could you please take a look? label Jul 24, 2020
@zier-one zier-one marked this pull request as ready for review July 24, 2020 07:10
@zier-one
Copy link
Contributor Author

/run-all-tests

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2020 •
edited
Loading

Codecov Report

Merging #787 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #787   +/-   ##
===========================================
  Coverage   31.7860%   31.7860%           
===========================================
  Files            98         98           
  Lines         11181      11181           
===========================================
  Hits           3554       3554           
  Misses         7278       7278           
  Partials        349        349

@zier-one zier-one changed the title chose one index to output delete events *: chose one index to output delete events Jul 24, 2020
@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one zier-one mentioned this pull request Jul 27, 2020
Merged
@amyangfei amyangfei added this to the v4.0.4 milestone Jul 28, 2020
@amyangfei
Copy link
Contributor

Please add a release note

@zier-one
Copy link
Contributor Author

Please add a release note

done

amyangfei
amyangfei reviewed Jul 28, 2020
View reviewed changes
cdc/model/sink.go Outdated Show resolved Hide resolved
cdc/entry/mounter.go Show resolved Hide resolved
cdc/entry/schema_storage.go Outdated Show resolved Hide resolved
cdc/entry/schema_storage.go Outdated
}
if handleIndexOffset >= 0 {
ti.HandleIndexID = ti.Indices[handleIndexOffset].ID
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to add fatal log here, if handle index is not found, we can record more information here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check it here:
https://github.com/pingcap/ticdc/pull/787/files#diff-8cae4fdc1c5f0a4e944025f65917425fR437

Copy link
Contributor

Choose a reason for hiding this comment

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

But the check in mounter can't log enough debug information

cdc/sink/dispatcher/table.go Outdated Show resolved Hide resolved
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2020
@amyangfei
Copy link
Contributor

/run-kafka-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor

/run-all-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

@amyangfei
Copy link
Contributor

/run-kafka-tests tidb=release-4.0 tikv=release-4.0 pd=release-4.0

@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

/run-all-tests

@zier-one zier-one added the release-blocker This issue blocks a release. Please solve it ASAP. label Aug 3, 2020
Merge remote-tracking branch 'pingcap/master' into delete_from_one_index
4c22abf 
# Conflicts:
#	cdc/entry/mounter.go
#	cdc/sink/dispatcher/default.go
@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

/run-all-tests

@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

/run-all-tests

@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

/run-all-tests

liuzix
liuzix reviewed Aug 3, 2020
View reviewed changes
Copy link
Contributor

@liuzix liuzix left a comment

Choose a reason for hiding this comment

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

The rest lgtm

cdc/model/sink.go Outdated Show resolved Hide resolved
cdc/model/sink.go Outdated Show resolved Hide resolved
cdc/entry/mounter.go Outdated Show resolved Hide resolved
cdc/entry/mounter.go Outdated Show resolved Hide resolved
cdc/model/schema_storage.go Outdated Show resolved Hide resolved
cdc/model/schema_storage.go Outdated Show resolved Hide resolved
cdc/model/schema_storage.go Outdated Show resolved Hide resolved
cdc/model/schema_storage.go Outdated Show resolved Hide resolved
cdc/model/sink.go Outdated Show resolved Hide resolved
@liuzix
Apply suggestions from code review
0f8623f 
Co-authored-by: Zixiong Liu <zl2683@columbia.edu>
@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

The rest lgtm

thanks a lot

@liuzix
Copy link
Contributor

liuzix commented Aug 3, 2020

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 3, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 3, 2020
@zier-one
Copy link
Contributor Author

zier-one commented Aug 3, 2020

/merge

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

Your auto merge job has been accepted, waiting for:

  • 815
  • 797

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit aa496b9 into pingcap:master Aug 3, 2020
@zier-one zier-one deleted the delete_from_one_index branch September 14, 2020 03:27
@hi-rustin hi-rustin mentioned this pull request Jul 15, 2021
Merged
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

@liuzix liuzix liuzix left review comments

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

Assignees
No one assigned
Labels
release-blocker This issue blocks a release. Please solve it ASAP. 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
v4.0.5
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zier-one @codecov-commenter @amyangfei @liuzix @ti-srebot