sorter/leveldb(ticdc): separate write from table sorter #4686

overvenus · 2022-02-24T07:41:03Z

What problem does this PR solve?

Issue Number: ref #4631

What is changed and how it works?

It separate write from table sorter.

Note, for now it's dead code, will be enabled in later PRs.

Check List

Tests

Unit test

Release note

None

Signed-off-by: Neil Shen <[email protected]>

ti-chi-bot · 2022-02-24T07:41:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

sdojjy
zhaoxinyu

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.

Signed-off-by: Neil Shen <[email protected]>

codecov-commenter · 2022-02-24T13:28:38Z

Codecov Report

Merging #4686 (fb1e5a7) into master (9607554) will decrease coverage by 0.5474%.
The diff coverage is 42.2577%.

Flag	Coverage Δ
cdc	`59.6327% <48.9983%> (-0.2896%)`	⬇️
dm	`51.2942% <32.2139%> (-0.7347%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #4686        +/-   ##
================================================
- Coverage   55.6402%   55.0927%   -0.5475%     
================================================
  Files           494        513        +19     
  Lines         61283      63718      +2435     
================================================
+ Hits          34098      35104      +1006     
- Misses        23750      25133      +1383     
- Partials       3435       3481        +46

sdojjy · 2022-02-25T02:10:45Z

cdc/sorter/leveldb/message/task.go

+	InputEvent *model.PolymorphicEvent
+	// Latest resolved ts / commit ts for readers.
+	// writer -> reader
+	ReadTs ReadTs


It makes the logic complex and unreadable to add so many fields in one struct, some fields are always nil in some cases.
And the Task is a part of Message, The size will be larger and larger, maybe we should find a better way to extend the Task or Message.

It's good point! Ideally tasks should be an enum type in rust, so tasks can be separated and more readable. Unfortunately, golang does not support it.

And the Task is a part of Message, The size will be larger and larger, maybe we should find a better way to extend the Task or Message.

Maybe we should make Task be a pointer in Message? Though it makes memory allocation, not sure if it helps performance.

Also see, notes in #4631

sdojjy · 2022-02-25T02:26:42Z

cdc/sorter/leveldb/system/system.go

@@ -68,10 +70,14 @@ func NewSystem(dir string, memPercentage float64, cfg *config.DBConfig) *System
 		WorkerNumber(cfg.Count).Build()
 	compactSystem, compactRouter := actor.NewSystemBuilder("sorter-compactor").
 		WorkerNumber(cfg.Count).Build()
+	writerSystem, writerRouter := actor.NewSystemBuilder("sorter-writer").


we have sorter-db and sorter-writer, can you add some comments about sorter-db

sdojjy · 2022-02-25T02:34:47Z

cdc/sorter/leveldb/writer.go

+		TableID: w.tableID,
+		ReadTs: message.ReadTs{
+			MaxCommitTs:   maxCommitTs,
+			MaxResolvedTs: maxResolvedTs,


maybe the write task is not finished, is it ok to send maxResolvedTs to the reader?

Yes, as reader/writer send tasks to the same leveldb, so tasks are ordered.
ReadTs will trigger reader to take iterator from leveldb, it happens after writer sends writes to leveldb.
And before leveldb takes iterators, it flushed all pending writes, so iterators see all writes.

sdojjy · 2022-02-25T02:36:01Z

cdc/sorter/leveldb/writer.go

+	"go.uber.org/zap"
+)
+
+// writer is a thin shim that batches, translates events into key vaule pairs


Suggested change

// writer is a thin shim that batches, translates events into key vaule pairs

// writer is a thin shim that batches, translates events into key-value pairs

cdc/sorter/leveldb/writer.go

Signed-off-by: Neil Shen <[email protected]>

zhaoxinyu · 2022-02-25T05:26:41Z

cdc/sorter/leveldb/writer.go

+
+	// Notify reader that there is something to read.
+	//
+	// It's ok to noify reader immediately without waiting writes done,


Suggested change

// It's ok to noify reader immediately without waiting writes done,

// It's ok to notify reader immediately without waiting writes done,

zhaoxinyu · 2022-02-25T05:27:21Z

/LGTM

sdojjy

lgtm

overvenus · 2022-02-25T07:21:57Z

/merge

ti-chi-bot · 2022-02-25T07:21:59Z

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

Commit hash: fb1e5a7

zhaoxinyu · 2022-02-25T07:46:42Z

TestMySQLSinkFlushResovledTs failed.

zhaoxinyu · 2022-02-25T07:47:07Z

/run-leak-test

overvenus · 2022-02-25T09:08:45Z

/merge

ti-chi-bot · 2022-02-25T09:09:01Z

@overvenus: 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.

overvenus · 2022-02-25T14:09:08Z

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

Signed-off-by: Neil Shen <[email protected]>

into leveldb-split-write

overvenus · 2022-02-26T01:15:01Z

/merge

ti-chi-bot · 2022-02-26T01:15:04Z

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

Commit hash: edeb755

overvenus · 2022-02-26T04:00:46Z

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

sorter/leveldb(ticdc): separate write from table sorter

ed47259

Signed-off-by: Neil Shen <[email protected]>

overvenus added subject/performance Denotes an issue or pull request is related to replication performance. component/sorter Sorter component. area/ticdc Issues or PRs related to TiCDC. labels Feb 24, 2022

overvenus requested review from sdojjy, zhaoxinyu and asddongmen February 24, 2022 07:41

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 24, 2022

address lints

65957c6

Signed-off-by: Neil Shen <[email protected]>

sdojjy reviewed Feb 25, 2022

View reviewed changes

zhaoxinyu reviewed Feb 25, 2022

View reviewed changes

cdc/sorter/leveldb/writer.go Show resolved Hide resolved

address comments

fb1e5a7

Signed-off-by: Neil Shen <[email protected]>

zhaoxinyu reviewed Feb 25, 2022

View reviewed changes

zhaoxinyu approved these changes Feb 25, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 25, 2022

sdojjy approved these changes Feb 25, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 25, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 25, 2022

Merge branch 'master' into leveldb-split-write

dda0efe

Merge branch 'master' into leveldb-split-write

e48970a

overvenus added 2 commits February 26, 2022 09:07

try fix pd breaking change

71be3b3

Signed-off-by: Neil Shen <[email protected]>

Merge branch 'leveldb-split-write' of https://github.com/overvenus/ticdc

edeb755

into leveldb-split-write

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 26, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 26, 2022

ti-chi-bot merged commit fa006db into pingcap:master Feb 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sorter/leveldb(ticdc): separate write from table sorter #4686

sorter/leveldb(ticdc): separate write from table sorter #4686

overvenus commented Feb 24, 2022

ti-chi-bot commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Feb 24, 2022 •

edited

Loading

sdojjy Feb 25, 2022

overvenus Feb 25, 2022

overvenus Feb 25, 2022

sdojjy Feb 25, 2022

sdojjy Feb 25, 2022

overvenus Feb 25, 2022

sdojjy Feb 25, 2022

zhaoxinyu Feb 25, 2022

zhaoxinyu commented Feb 25, 2022

sdojjy left a comment

overvenus commented Feb 25, 2022

ti-chi-bot commented Feb 25, 2022

zhaoxinyu commented Feb 25, 2022

zhaoxinyu commented Feb 25, 2022

overvenus commented Feb 25, 2022

ti-chi-bot commented Feb 25, 2022

overvenus commented Feb 25, 2022

overvenus commented Feb 26, 2022

ti-chi-bot commented Feb 26, 2022

overvenus commented Feb 26, 2022

	// writer is a thin shim that batches, translates events into key vaule pairs
	// writer is a thin shim that batches, translates events into key-value pairs

	// It's ok to noify reader immediately without waiting writes done,
	// It's ok to notify reader immediately without waiting writes done,

sorter/leveldb(ticdc): separate write from table sorter #4686

sorter/leveldb(ticdc): separate write from table sorter #4686

Conversation

overvenus commented Feb 24, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 24, 2022 • edited Loading

codecov-commenter commented Feb 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoxinyu commented Feb 25, 2022

sdojjy left a comment

Choose a reason for hiding this comment

overvenus commented Feb 25, 2022

ti-chi-bot commented Feb 25, 2022

zhaoxinyu commented Feb 25, 2022

zhaoxinyu commented Feb 25, 2022

overvenus commented Feb 25, 2022

ti-chi-bot commented Feb 25, 2022

overvenus commented Feb 25, 2022

overvenus commented Feb 26, 2022

ti-chi-bot commented Feb 26, 2022

overvenus commented Feb 26, 2022

ti-chi-bot commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Feb 24, 2022 •

edited

Loading