-
Notifications
You must be signed in to change notification settings - Fork 289
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.
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
sink(ticdc): set max-message-bytes default to 10m #4036
Merged
ti-chi-bot
merged 19 commits into
pingcap:master
from
3AceShowHand:max-message-bytes-10m
Dec 24, 2021
Merged
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
6cbbfd8
change default to 10m
3AceShowHand d2da2d3
set the default to 10m
3AceShowHand 7392030
update test.
3AceShowHand ca894b4
update test case
3AceShowHand b5b277a
update test case
3AceShowHand 234296c
Rename encode variable name.
3AceShowHand eb90bc7
refine the log.
3AceShowHand f604c27
update go mod.
3AceShowHand 7dff35f
fix typo.
3AceShowHand a4f934d
fix typo.
3AceShowHand 1e7f317
fix unit test.
3AceShowHand c777cf0
move to config.
3AceShowHand 2587ec6
make check.
3AceShowHand 155eded
fix make check.
3AceShowHand c459932
Update cdc/sink/codec/craft_test.go
3AceShowHand 915177c
Update cdc/sink/codec/json_test.go
3AceShowHand 02cdcd0
Update pkg/config/sink.go
3AceShowHand a1c0a4c
fix lint.
3AceShowHand 285fb1d
Merge branch 'master' into max-message-bytes-10m
ti-chi-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This func seems strange at the moment. Can we split it up?
This way we won't have to pass two configurations and modify them at the same time. Now this function has become very complicated. Originally we didn't modify Sarama's configuration in this method. Now it is not only responsible for creating the topic, but also for setting the sarma configuration correctly, but this configuration is not directly related to creating the topic(It will affect when syncing, but it may be a pre-condition. So I think it can be separated). It does too many things. I prefer to keep them separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a better way to get it not to set it up at the same time that would work too. I can only think of separating it at the moment.
The sarma configuration has been patched once above via kafka's configuration, but we're modifying it at the same time in this function.
It is currently confusing from either the caller's or the test code's point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worthy another PR to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to do it in this time. It should just be a simple split method will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it would be better to do this after the release, to prevent some potential new problems.