ticdc: fix detecting kafka version #11048

wk989898 · 2024-05-08T02:40:53Z

What problem does this PR solve?

Issue Number: close #11047

What is changed and how it works?

Expose requestVersion param and use the low version by default.
After getting version information, set kafka version as high as possible.

Check List

Tests

Unit test
Integration test
Manual test

start cluster and Kafka (version=3.7.0)
create a changefeed with kafka protocol

bin/cdc cli changefeed create -c test --sink-uri "kafka://127.0.0.1:9092/test?topic=test&protocol=open-protocol&max-message-bytes=41943040&compressionType=ZLIB&kafka-version=0.11.0.0"

check CDC log

[WARN] [sarama.go:139] ["The Kafka version you assigned may not be correct. Please assign a version equal to or less than the specified version"] [assignedVersion=0.11.0.0] [desiredVersion=2.8.0]

note: The max Kafka version which sarama detects is v2.8.0.

kafka version	status
3.7.0	success
3.0.1	success
2.8.0	success
2.7.0	success
2.5.0	success
2.4.0	success
2.3.1	success
2.1.1	success
2.1.0	success
2.0.1	success
2.0.0	success
1.0.1	success
0.11.0	success

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

None

ti-chi-bot · 2024-05-08T02:41:02Z

Hi @wk989898. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

asddongmen · 2024-05-16T08:09:51Z

Please update this section in the PR description:

Check List
Tests
Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

CharlesCheung96 · 2024-05-16T08:23:02Z

pkg/sink/kafka/sarama.go

 		return KafkaVersion, err
 	}
-	apiResponse, err := broker.ApiVersions(&sarama.ApiVersionsRequest{Version: 3})
+	apiResponse, err := broker.ApiVersions(&sarama.ApiVersionsRequest{Version: requestVersion})


Please explain the detailed behavior with different requestVersion.

mockbroker only works when version is equal 3, this version is not supported for
low version kafka, so this change always uses the lowest version 0 to connect

CharlesCheung96 · 2024-05-16T08:24:08Z

pkg/sink/kafka/options.go

@@ -151,6 +151,7 @@ type Options struct {
 	ReplicationFactor int16
 	Version           string
 	IsAssignedVersion bool
+	RequestVersion    int16


Do we need to verify the value range of the RequestVersion?

No, it only use in unit test

Then, maybe we should use a const variable and set it in only in tests.

it will be 0 as default, and the unit test needs to be 3.

CharlesCheung96 · 2024-05-16T08:24:33Z

Please add the test result in pr body.

CharlesCheung96 · 2024-05-16T08:24:52Z

/test all

codecov · 2024-05-16T08:46:35Z

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 57.7521%. Comparing base (566c77c) to head (fb8c124).
Report is 21 commits behind head on master.

Additional details and impacted files

Components	Coverage Δ
cdc	`62.2394% <33.3333%> (+0.6400%)`	⬆️
dm	`51.3640% <ø> (+0.1486%)`	⬆️
engine	`63.4585% <ø> (+0.0706%)`	⬆️

Flag	Coverage Δ
unit	`57.7521% <33.3333%> (+0.3916%)`	⬆️

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

@@               Coverage Diff                @@
##             master     #11048        +/-   ##
================================================
+ Coverage   57.3604%   57.7521%   +0.3916%     
================================================
  Files           851        854         +3     
  Lines        125590     126475       +885     
================================================
+ Hits          72039      73042      +1003     
+ Misses        48130      48054        -76     
+ Partials       5421       5379        -42

wk989898 · 2024-05-16T09:56:03Z

We need to consider default-kafka-version. It should be set to the lowest version supported by TiCDC. @3AceShowHand @flowbehappy

tiflow/pkg/sink/kafka/sarama.go

Line 32 in 1c7c9ae

defaultKafkaVersion = sarama.V2_0_0_0

wk989898 · 2024-05-16T10:30:54Z

/retest

ti-chi-bot · 2024-05-16T10:31:10Z

@wk989898: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 kubernetes/test-infra repository.

ti-chi-bot · 2024-05-17T03:05:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3AceShowHand, CharlesCheung96

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [3AceShowHand,CharlesCheung96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-05-17T03:06:00Z

[LGTM Timeline notifier]

Timeline:

2024-05-16 07:50:18.395616914 +0000 UTC m=+1725972.152752487: ☑️ agreed by 3AceShowHand.
2024-05-17 03:05:59.429382655 +0000 UTC m=+1795313.186518228: ☑️ agreed by CharlesCheung96.

wk989898 · 2024-05-17T03:34:48Z

/retest

ti-chi-bot · 2024-05-17T03:35:03Z

@wk989898: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 kubernetes/test-infra repository.

CharlesCheung96 · 2024-05-17T03:42:31Z

/retest

CharlesCheung96 · 2024-05-17T05:52:22Z

/retest

CharlesCheung96 · 2024-05-17T07:45:45Z

/retest

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

ti-chi-bot · 2024-05-17T08:15:22Z

In response to a cherrypick label: new pull request created to branch release-7.5: #11136.

ti-chi-bot · 2024-05-17T08:16:01Z

In response to a cherrypick label: new pull request created to branch release-8.1: #11137.

close pingcap#11047

close #11047

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

close #11047

wk989898 added 10 commits March 27, 2024 15:49

feat: add kafka version check

8f7849b

fix typo

1b02cc0

fix typo

b075719

modify func and code style

57a178c

update

5cb0d36

fix unused param check

c397631

update test

fc56778

fix no available brokers

49f78fb

Merge branch 'pingcap:master' into kafka-version

4c10b56

update detecting kafka version

2a287f7

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed contribution Indicates that the PR was contributed by an external member. needs-ok-to-test labels May 8, 2024

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2024

Merge remote-tracking branch 'tiflow/master' into kafka-version

cea715c

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2024

update

fb8c124

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label May 14, 2024

3AceShowHand approved these changes May 16, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm approved labels May 16, 2024

CharlesCheung96 reviewed May 16, 2024

View reviewed changes

ti-chi-bot added the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label May 16, 2024

ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label May 16, 2024

CharlesCheung96 approved these changes May 17, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels May 17, 2024

ti-chi-bot bot merged commit ab6a9f9 into pingcap:master May 17, 2024
28 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request May 17, 2024

This is an automated cherry-pick of pingcap#11048

7fc2dc6

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

ti-chi-bot mentioned this pull request May 17, 2024

ticdc: fix detecting kafka version (#11048) #11136

Merged

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request May 17, 2024

ticdc: fix detecting kafka version (pingcap#11048)

e8fc06e

close pingcap#11047

ti-chi-bot mentioned this pull request May 17, 2024

ticdc: fix detecting kafka version (#11048) #11137

Merged

ti-chi-bot bot pushed a commit that referenced this pull request May 17, 2024

ticdc: fix detecting kafka version (#11048) (#11137)

15943cb

close #11047

wk989898 added a commit to ti-chi-bot/tiflow that referenced this pull request May 17, 2024

This is an automated cherry-pick of pingcap#11048

1dab194

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

ti-chi-bot bot pushed a commit that referenced this pull request May 22, 2024

ticdc: fix detecting kafka version (#11048) (#11136)

126365e

close #11047

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ticdc: fix detecting kafka version #11048

ticdc: fix detecting kafka version #11048

wk989898 commented May 8, 2024 •

edited

ti-chi-bot bot commented May 8, 2024

asddongmen commented May 16, 2024

CharlesCheung96 May 16, 2024

wk989898 May 16, 2024

CharlesCheung96 May 16, 2024

wk989898 May 16, 2024

CharlesCheung96 May 16, 2024

wk989898 May 17, 2024

CharlesCheung96 commented May 16, 2024

CharlesCheung96 commented May 16, 2024

codecov bot commented May 16, 2024 •

edited

wk989898 commented May 16, 2024

wk989898 commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

wk989898 commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

CharlesCheung96 commented May 17, 2024

CharlesCheung96 commented May 17, 2024

CharlesCheung96 commented May 17, 2024

ti-chi-bot commented May 17, 2024

ti-chi-bot commented May 17, 2024

ticdc: fix detecting kafka version #11048

ticdc: fix detecting kafka version #11048

Conversation

wk989898 commented May 8, 2024 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot bot commented May 8, 2024

asddongmen commented May 16, 2024

CharlesCheung96 May 16, 2024

Choose a reason for hiding this comment

wk989898 May 16, 2024

Choose a reason for hiding this comment

CharlesCheung96 May 16, 2024

Choose a reason for hiding this comment

wk989898 May 16, 2024

Choose a reason for hiding this comment

CharlesCheung96 May 16, 2024

Choose a reason for hiding this comment

wk989898 May 17, 2024

Choose a reason for hiding this comment

CharlesCheung96 commented May 16, 2024

CharlesCheung96 commented May 16, 2024

codecov bot commented May 16, 2024 • edited

Codecov Report

wk989898 commented May 16, 2024

wk989898 commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

[LGTM Timeline notifier]

wk989898 commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

CharlesCheung96 commented May 17, 2024

CharlesCheung96 commented May 17, 2024

CharlesCheung96 commented May 17, 2024

ti-chi-bot commented May 17, 2024

ti-chi-bot commented May 17, 2024

wk989898 commented May 8, 2024 •

edited

codecov bot commented May 16, 2024 •

edited