sink(ticdc): set max-message-bytes default to 10m #4036

3AceShowHand · 2021-12-23T08:47:31Z

What problem does this PR solve?

close #4041
set Kafka producer's default max-message-bytes to 10M, and use the minimum value among sink-uri's max-message-bytes, broker's message.max.bytes, topic's max.message.bytes to initialize the producer.

What is changed and how it works?

set the default value of max-message-bytes to 10M
use the min value of the 3 mentioned above to initialize the producer.

Check List

Tests

Unit test

Code changes

Side effects

Related changes

Release note

Set `max-message-bytes` default to 10M, and use the min value with topic and broker to initialize the producer.

ti-chi-bot · 2021-12-23T08:47:32Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

amyangfei
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.

3AceShowHand · 2021-12-23T09:48:14Z

/run-all-tests

cdc/sink/codec/json.go

cdc/sink/producer/kafka/kafka.go

cdc/sink/codec/json.go

cdc/sink/producer/kafka/kafka_test.go

amyangfei

rest lgtm

3AceShowHand · 2021-12-23T11:17:48Z

/run-all-tests

codecov-commenter · 2021-12-23T11:53:29Z

Codecov Report

Merging #4036 (a1c0a4c) into master (3873d39) will decrease coverage by 1.7871%.
The diff coverage is 67.3196%.

Flag	Coverage Δ
cdc	`58.8141% <67.3196%> (+0.5775%)`	⬆️
dm	`52.2983% <ø> (-3.7363%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #4036        +/-   ##
================================================
- Coverage   57.0741%   55.2870%   -1.7872%     
================================================
  Files           478        486         +8     
  Lines         56551      60043      +3492     
================================================
+ Hits          32276      33196       +920     
- Misses        20978      23498      +2520     
- Partials       3297       3349        +52

cdc/sink/codec/craft_test.go

cdc/sink/codec/json_test.go

pkg/config/sink.go

Rustin170506 · 2021-12-23T14:13:48Z

cdc/sink/producer/kafka/kafka.go

@@ -345,7 +345,7 @@ func kafkaClientID(role, captureAddr, changefeedID, configuredClientID string) (
 	return
 }

-func validateMaxMessageBytesAndCreateTopic(admin kafka.ClusterAdminClient, topic string, config *Config) error {
+func validateMaxMessageBytesAndCreateTopic(admin kafka.ClusterAdminClient, topic string, config *Config, saramaConfig *sarama.Config) error {


This func seems strange at the moment. Can we split it up?

getMaxMessageBytes is responsible for getting the right value

createTopic is used to create the topic

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.

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.

This worthy another PR to do it.

I think it might be better to do it in this time. It should just be a simple split method will work.

I still think it would be better to do this after the release, to prevent some potential new problems.

overvenus · 2021-12-24T08:16:49Z

/merge

ti-chi-bot · 2021-12-24T08:16:51Z

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

Commit hash: a1c0a4c

ti-chi-bot · 2021-12-24T08:16:58Z

@3AceShowHand: 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.

ti-chi-bot · 2021-12-24T08:54:24Z

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