Skip to content

sink & mounter: refine the struct of row changed events#830

Merged
ti-srebot merged 15 commits into
pingcap:masterfrom
zier-one:new_row_changed_event
Aug 12, 2020
Merged

sink & mounter: refine the struct of row changed events#830
ti-srebot merged 15 commits into
pingcap:masterfrom
zier-one:new_row_changed_event

Conversation

@zier-one
Copy link
Copy Markdown
Contributor

@zier-one zier-one commented Aug 10, 2020 •
edited by liuzix
Loading

What problem does this PR solve?

the new struct of row change events

// RowChangedEvent represents a row changed event
type RowChangedEvent struct {
	StartTs  uint64 `json:"start-ts"`
	CommitTs uint64 `json:"commit-ts"`

	RowID int64 `json:"row-id"`

	Table *TableName `json:"table"`

	// TODO: remove it
	Delete bool `json:"delete"`

	TableInfoVersion uint64 `json:"table-info-version,omitempty"`

	// TODO: remove it
	// if the table of this row only has one unique index(includes primary key),
	// IndieMarkCol will be set to the name of the unique index
	IndieMarkCol string `json:"indie-mark-col"`

	Columns    []*Column `json:"columns"`
	PreColumns []*Column `json:"pre-columns"`

	IndexColumns [][]int

	// TODO: remove it
	Keys []string `json:"keys"`
}
  • Delete, IndieMarkCol, Keys will be removed in next PR
  • To maintain column order stability, use slices to store Columns and PreColumns
    • the indices of the Columns are fixed. If a row event has only partial columns, the missing columns are filled using nil. for example, a complete event like :RowChangedEvent{Columns:[col1,col2,col3]}, in the event of deletion, cdc only gets column 3, like:RowChangedEvent{Columns:[nil,nil,col3]}.

Check List

Tests

  • Unit test
  • Integration test

Release note

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 10, 2020

/run-integration-tests

@zier-one zier-one changed the title New row changed event sink & mounter: refine the struct of row changed events Aug 10, 2020
@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 10, 2020

/run-integration-tests

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 10, 2020

/run-integration-tests

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 10, 2020

/run-integration-tests

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 11, 2020

/run-integration-tests

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 11, 2020

/run-integration-tests

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 11, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master       #830   +/-   ##
===========================================
  Coverage   33.2309%   33.2309%           
===========================================
  Files            97         97           
  Lines         11390      11390           
===========================================
  Hits           3785       3785           
  Misses         7253       7253           
  Partials        352        352

@zier-one zier-one marked this pull request as ready for review August 11, 2020 06:49
@zier-one zier-one added the status/ptal Could you please take a look? label Aug 11, 2020
@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 11, 2020

/run-all-tests

@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 11, 2020

/run-kafka-tests

liuzix
liuzix reviewed Aug 11, 2020
View reviewed changes
Copy link
Copy Markdown
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

Comment thread cdc/model/schema_storage.go Outdated
Comment thread cdc/model/schema_storage.go Outdated
Comment thread cdc/model/schema_storage.go Outdated
@zier-one
Copy link
Copy Markdown
Contributor Author

zier-one commented Aug 11, 2020

@liuzix PTAL again

@liuzix
Copy link
Copy Markdown
Contributor

liuzix commented Aug 11, 2020

/lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 11, 2020
amyangfei
amyangfei reviewed Aug 12, 2020
View reviewed changes
Comment thread cdc/model/schema_storage.go Outdated
}
}
if idx.Primary || idx.Unique {
if ti.IsIndexUnique(idx, true) {
Copy link
Copy Markdown
Contributor

@amyangfei amyangfei Aug 12, 2020

Choose a reason for hiding this comment

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

suggest to use another function for the condition of allowColumnNullable=true

Copy link
Copy Markdown
Contributor Author

@zier-one zier-one Aug 12, 2020

Choose a reason for hiding this comment

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

fine

Comment thread cdc/model/schema_storage.go
Comment on lines +212 to +224
flag := ti.ColumnsFlag[colInfo.ID]
if idxInfo.Primary {
flag.SetIsPrimaryKey()
}
if idxInfo.Unique {
flag.SetIsUniqueKey()
}
if len(idxInfo.Columns) > 1 {
flag.SetIsMultipleKey()
}
if idxInfo.ID == ti.HandleIndexID && ti.HandleIndexID >= 0 {
flag.SetIsHandleKey()
}
Copy link
Copy Markdown
Contributor

@amyangfei amyangfei Aug 12, 2020

Choose a reason for hiding this comment

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

Seems some flags including PrimaryKey, UniqueKey and IsHandleKey have been set in L184-L195

Copy link
Copy Markdown
Contributor Author

@zier-one zier-one Aug 12, 2020

Choose a reason for hiding this comment

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

yes, some flags will be set twice, but the final result will not be affected.

amyangfei
amyangfei reviewed Aug 12, 2020
View reviewed changes
Copy link
Copy Markdown
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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 12, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 12, 2020
@amyangfei
Copy link
Copy Markdown
Contributor

amyangfei commented Aug 12, 2020

/merge

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

ti-srebot commented Aug 12, 2020

/run-all-tests

@ti-srebot ti-srebot merged commit 8bfe97b into pingcap:master Aug 12, 2020
@zier-one zier-one deleted the new_row_changed_event branch September 14, 2020 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

+2 more reviewers

@amyangfei amyangfei amyangfei left review comments

@liuzix liuzix liuzix left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

5 participants

@zier-one @codecov-commenter @liuzix @amyangfei @ti-srebot