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

codec(cdc): fix encoder max-message-bytes #4074

Merged
merged 11 commits into from
Dec 26, 2021
Merged

codec(cdc): fix encoder max-message-bytes #4074

merged 11 commits into from
Dec 26, 2021

Conversation

3AceShowHand
Copy link
Contributor

@3AceShowHand 3AceShowHand commented Dec 26, 2021 •
edited
Loading

What problem does this PR solve?

close #4089

when initializing the encoder, make sure max-message-bytes in the opts, and set identical to producer's configuration.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

Release note 

`None`.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 26, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • overvenus

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.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 26, 2021
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2021 •
edited
Loading

Codecov Report

Merging #4074 (913e2d5) into master (3873d39) will decrease coverage by 1.9284%.
The diff coverage is 66.6382%.

Flag Coverage Δ
cdc 58.5995% <66.6382%> (+0.3629%) ⬆️
dm 52.2183% <ø> (-3.8163%) ⬇️

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

@@               Coverage Diff                @@
##             master      #4074        +/-   ##
================================================
- Coverage   57.0741%   55.1457%   -1.9285%     
================================================
  Files           478        486         +8     
  Lines         56551      60050      +3499     
================================================
+ Hits          32276      33115       +839     
- Misses        20978      23568      +2590     
- Partials       3297       3367        +70

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 26, 2021
@3AceShowHand
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 26, 2021
@3AceShowHand
Copy link
Contributor Author

/run-all-tests

@3AceShowHand
Copy link
Contributor Author

/run-leak-test

@3AceShowHand
Copy link
Contributor Author

/run-all-tests

amyangfei
amyangfei reviewed Dec 26, 2021
View reviewed changes
cdc/sink/codec/craft.go Outdated Show resolved Hide resolved
cdc/sink/codec/craft.go Outdated Show resolved Hide resolved
@amyangfei
Copy link
Contributor

Need to confirm:

  • Is cherry pick to release branches needed
  • whether release note is needed? (a new introduced bug or an old bug)

amyangfei
amyangfei reviewed Dec 26, 2021
View reviewed changes
cdc/sink/codec/json.go
Comment on lines -618 to -623
d.maxMessageBytes = config.DefaultMaxMessageBytes
if maxMessageBytes, ok := params["max-message-bytes"]; ok {
d.maxMessageBytes, err = strconv.Atoi(maxMessageBytes)
if err != nil {
return cerror.ErrSinkInvalidConfig.Wrap(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The original logic is not elegant, but what is the bug in it?

  • If max-message-bytes is set in sink-uri, the following lines set it:

    tiflow/cdc/sink/mq.go

    Lines 419 to 422 in 43a599d

    s = sinkURI.Query().Get("max-message-bytes")
    if s != "" {
    opts["max-message-bytes"] = s
    }
  • If max-message-bytes is not set in sink-uri, config.DefaultMaxMessageBytes will be used 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.

we have to update the config once more.because we would query Kafka broker to get topicMaxMessageBytes or brokerMessageMaxBytes, and then choose the minimum of min(max-message-bytes, topicMaxMessageBytes, brokerMessageMaxBytes)

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense, maybe more scenario tests can be added, either in integration test or test infra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have to refact the configuration in the kafka, Config, sarama.Config, opts, and cover the adjust logic.

3AceShowHand and others added 2 commits December 26, 2021 17:45
@3AceShowHand @amyangfei
Update cdc/sink/codec/craft.go
c817de0 
Co-authored-by: amyangfei <amyangfei@gmail.com>
@3AceShowHand @amyangfei
Update cdc/sink/codec/craft.go
aade392 
Co-authored-by: amyangfei <amyangfei@gmail.com>
@3AceShowHand 3AceShowHand added needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. labels Dec 26, 2021
@3AceShowHand
Copy link
Contributor Author

/run-all-tests

@3AceShowHand
Copy link
Contributor Author

/run-leak-test

@3AceShowHand
Copy link
Contributor Author

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

@3AceShowHand
Copy link
Contributor Author

/run-kafka-integration-test

@3AceShowHand 3AceShowHand merged commit f097a12 into pingcap:master Dec 26, 2021
ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Dec 26, 2021
@3AceShowHand @ti-chi-bot
This is an automated cherry-pick of pingcap#4074
9afa828 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 26, 2021
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #4075.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Dec 26, 2021
@3AceShowHand @ti-chi-bot
This is an automated cherry-pick of pingcap#4074
1bc3559 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 26, 2021
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #4076.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Dec 26, 2021
@3AceShowHand @ti-chi-bot
This is an automated cherry-pick of pingcap#4074
afb5392 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 26, 2021
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #4077.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Dec 26, 2021
@3AceShowHand @ti-chi-bot
This is an automated cherry-pick of pingcap#4074
42dc87b 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 26, 2021
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #4078.

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #4079.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Dec 26, 2021
@3AceShowHand @ti-chi-bot
This is an automated cherry-pick of pingcap#4074
a01549d 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 26, 2021
Closed
3AceShowHand added a commit that referenced this pull request Dec 26, 2021
@ti-chi-bot @3AceShowHand
codec(cdc): fix encoder max-message-bytes (#4074) (#4076)
39ebddf 
* This is an automated cherry-pick of #4074

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

* remove craft.

* fix json.

* fix more.

* fix mq test.

* fix json test.

* fix mq test.

* fix kafka test.

* fix json test.

* fix kafka test

* fix kafka test

* fix kafka test

* comment out.

* revert comment.

* update go sum.

* move opt out.

Co-authored-by: Ling Jin <7138436+3AceShowHand@users.noreply.github.com>
Co-authored-by: 3AceShowHand <jinl1037@hotmail.com>
ti-chi-bot added a commit that referenced this pull request Dec 27, 2021
@ti-chi-bot
codec(cdc): fix encoder max-message-bytes (#4074) (#4077)
d94cf7c
zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Dec 29, 2021
@3AceShowHand @amyangfei @zhaoxinyu
codec(cdc): fix encoder max-message-bytes (pingcap#4074)
2737fff 
* fix encoder.

* add test.

* fix test.

* fix encoder SetParams.

* fix test in json.

* update craft test.

* fix craft test.

* Refine error.

* Update cdc/sink/codec/craft.go

Co-authored-by: amyangfei <amyangfei@gmail.com>

* Update cdc/sink/codec/craft.go

Co-authored-by: amyangfei <amyangfei@gmail.com>

* fix test in craft.

Co-authored-by: amyangfei <amyangfei@gmail.com>
overvenus pushed a commit that referenced this pull request Jan 24, 2022
@ti-chi-bot @3AceShowHand
sink(ticdc): set max-message-bytes default to 10m (#4036) (#4063)
c13ec95 
* This is an automated cherry-pick of #4036

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

* remove config.

* fix kafka.

* This is an automated cherry-pick of #4074

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

* Resolve mq.

* remove unncessary mock impl.

* try fix .

* resolve conflict.

Co-authored-by: Ling Jin <7138436+3AceShowHand@users.noreply.github.com>
Co-authored-by: 3AceShowHand <jinl1037@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overvenus overvenus overvenus approved these changes

@Rustin170506 Rustin170506 Awaiting requested review from Rustin170506

@amyangfei amyangfei Awaiting requested review from amyangfei

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cdc encoder max-message-bytes is not set correctly.
5 participants
@3AceShowHand @ti-chi-bot @codecov-commenter @amyangfei @overvenus