Skip to content

[improve][io] Upgrade Kafka client, connect runtime to 2.8.2 and Confluent version to 6.2.8 #19010

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
Merged
lhotari merged 4 commits into from
Dec 21, 2022
Merged

[improve][io] Upgrade Kafka client, connect runtime to 2.8.2 and Confluent version to 6.2.8 #19010

lhotari merged 4 commits into from
Dec 21, 2022

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Dec 20, 2022 •
edited
Loading

Motivation

There are multiple Confluent platform versions in use at the moment.
There's now references to both version 7.0.1 and version 5.3.0 (from July 2019) and this doesn't make much sense.

The Confluent platform version should be compatible with the Kafka client and Kafka connect runtime versions.

Confluent 6.2.x is compatible with Kafka client 2.8.2 . (release notes of Confluent 6.2.8)

Modifications

  • Upgrade Kafka client and Kafka connect runtime to 2.8.2
  • Upgrade Confluent version to 6.2.8

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: lhotari#117

@lhotari
[improve][io] Upgrade Kafka client, connect runtime to 2.8.2 and Conf…
0618d89 
…luent version to 6.2.8

- Confluent 6.2.x is compatible with Kafka client 2.8.2
@lhotari lhotari added this to the 2.12.0 milestone Dec 20, 2022
@lhotari lhotari self-assigned this Dec 20, 2022
@lhotari lhotari mentioned this pull request Dec 20, 2022
Closed
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 20, 2022
@lhotari lhotari mentioned this pull request Dec 20, 2022
Merged
4 tasks
michaeljmarshall
michaeljmarshall approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM. Left a minor question.

pom.xml Outdated
Comment on lines 191 to 192
<kafka.confluent.schemaregistryclient.version>${confluent.version}</kafka.confluent.schemaregistryclient.version>
<kafka.confluent.avroserializer.version>${confluent.version}</kafka.confluent.avroserializer.version>
Copy link
Member

@michaeljmarshall michaeljmarshall Dec 20, 2022

Choose a reason for hiding this comment

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

Is it worth removing these variables entirely and replacing them with confluent.version?

Copy link
Member Author

@lhotari lhotari Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I guess so. I'll wait for options from other reviewers before making the change. @dlg99 What do you recommend?

Copy link
Contributor

@dlg99 dlg99 Dec 20, 2022

Choose a reason for hiding this comment

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

TBH I don't know why these are different.

Copy link
Contributor

@aymkhalil aymkhalil Dec 20, 2022

Choose a reason for hiding this comment

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

FYI the integ tests use v6.0.1 - not sure if it makes sense to reconcile their version in this PR but we are not testing against 6.2.x atm:

pulsar/tests/integration/src/test/java/org/apache/pulsar/tests/integration/io/sources/AvroKafkaSourceTest.java

Line 483 in 4e29a1e

public static final String CONFLUENT_PLATFORM_VERSION = "6.0.1";

Copy link
Member Author

@lhotari lhotari Dec 20, 2022

Choose a reason for hiding this comment

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

Thanks for the review feedback @dlg99 and @aymkhalil . PTAL

@lhotari
Copy link
Member Author

lhotari commented Dec 20, 2022

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022 •
edited
Loading

Codecov Report

Merging #19010 (2cca77d) into master (22866bd) will increase coverage by 1.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19010      +/-   ##
============================================
+ Coverage     45.92%   47.07%   +1.14%     
+ Complexity    10104     9380     -724     
============================================
  Files           680      626      -54     
  Lines         66758    59266    -7492     
  Branches       7147     6175     -972     
============================================
- Hits          30660    27900    -2760     
+ Misses        32680    28302    -4378     
+ Partials       3418     3064     -354
Flag Coverage Δ
unittests 47.07% <ø> (+1.14%) ⬆️

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

Impacted Files Coverage Δ
...ar/broker/loadbalance/impl/BundleSplitterTask.java 60.00% <0.00%> (-20.00%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
...che/pulsar/broker/intercept/BrokerInterceptor.java 0.00% <0.00%> (-13.05%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 68.51% <0.00%> (-9.26%) ⬇️
...ker/loadbalance/impl/LeastLongTermMessageRate.java 84.44% <0.00%> (-8.89%) ⬇️
...roker/service/persistent/MessageDeduplication.java 43.23% <0.00%> (-7.43%) ⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java 78.04% <0.00%> (-7.32%) ⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java 58.22% <0.00%> (-7.22%) ⬇️
...tent/PersistentDispatcherSingleActiveConsumer.java 54.54% <0.00%> (-6.59%) ⬇️
...balance/impl/SimpleResourceAllocationPolicies.java 48.57% <0.00%> (-5.72%) ⬇️
... and 145 more

tisonkun
tisonkun approved these changes Dec 21, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

OWASP warnings seem about other dependencies.

@lhotari lhotari merged commit 08591d9 into apache:master Dec 21, 2022
tisonkun
tisonkun reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@lhotari I'm wondering whether this change can help us get rid of the kafka-connect-avro-converter-shaded hack. Since we now already use the same avro version for pulsar-io-kafka-connect-adaptor (kafka-connect-avro-converter-shaded's only dependent).

Cross refer to the original PR #6034

@tisonkun tisonkun mentioned this pull request Feb 9, 2023
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tisonkun tisonkun tisonkun approved these changes

@dlg99 dlg99 dlg99 approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@eolivelli eolivelli Awaiting requested review from eolivelli

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

@freeznet freeznet Awaiting requested review from freeznet

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

+1 more reviewer

@aymkhalil aymkhalil aymkhalil approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lhotari lhotari

Labels

area/connector doc-not-needed Your PR changes do not impact docs

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@lhotari @codecov-commenter @dlg99 @aymkhalil @tisonkun @michaeljmarshall