Updates Kafka client from 2.8.1 to 3.3.1#130
Merged
andsel merged 9 commits intoDec 16, 2022
Merged
Conversation
Kafka 3.3.1 correspond to Confluent Platform 7.3.0. This commit - updates the jar dependencies to match version 3.3.1 and all relates scripts where the version is used. - with Kafka 3 the zookeper param was removed from kafka-topics.sh CLI tool, being replaced with bootstrap-server. - In Kafka 3 the "default" value was removed from the possibile values of "client.dns.lookup" setting, so plugin's input and output corresponding "client_dns_lookup" was consequently aligned.
Contributor
Author
|
The CI error that's happening on happens also with With LS Now, the class that can't be found Kafka Client switched
|
Contributor
Author
The full exception isWhich is summarized as: So it comes from |
Contributor
Author
|
Looking better at the problem, it originates from the fact that The |
karenzone
reviewed
Nov 29, 2022
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
karenzone
reviewed
Nov 29, 2022
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
…s consistently the TravisCI definitions
andsel
force-pushed
the
fix/update_kafka_version_3.3.1
branch
from
December 12, 2022 09:56
7e6e55c to
10b3ac5
Compare
…ed jruby's bundler from Logstash's artifacts
andsel
commented
Dec 13, 2022
robbavey
requested changes
Dec 13, 2022
| # connection. If the value is `resolve_canonical_bootstrap_servers_only` each entry will be | ||
| # resolved and expanded into a list of canonical names. | ||
| config :client_dns_lookup, :validate => ["default", "use_all_dns_ips", "resolve_canonical_bootstrap_servers_only"], :default => "default" | ||
| config :client_dns_lookup, :validate => ["use_all_dns_ips", "resolve_canonical_bootstrap_servers_only"], :default => "use_all_dns_ips" |
Member
There was a problem hiding this comment.
What did default previously map to in Kafka?
By not allowing the setting of default, this is potentially a breaking change (even though it is somewhat unlikely that a user will explicitly set default, it's a valid setting). If we choose to remove the explicit default value we need to flag it in change logs and release notes, and figure out how we choose to bundle this with future logstashes. We would also need to update the docs for the input and output
Contributor
Author
There was a problem hiding this comment.
I agree with you on the breaking change. Given the explanation in previous comment do you think that maybe is better if we implicitly remap default to use_all_dns_ips?. So that if one user has explicitly set default we switch implicitly to use_all_dns_ips. In worst case we let them to connect to another node while the previous setting would have resulted in connection error.
Member
There was a problem hiding this comment.
I think that is reasonable - we should add a note in the docs and the release notes to explicitly state the change, and they are changing the value of default to be the kafka default setting, rather than the explicit default value that Kafka previously used.
Contributor
Author
There was a problem hiding this comment.
Applied with commit b2568f0, so no anymore breaking change if the user uses default but just log a warn message.
Contributor
Author
There was a problem hiding this comment.
@robbavey I've removed the breaking change on the removal of default value by admitting it, deprecating and forcing to use_all_dns_ips.
The PR is read for another review, please 🙏
…e_all_dns_ips' if configured.
andsel
force-pushed
the
fix/update_kafka_version_3.3.1
branch
from
December 14, 2022 10:17
4ce293c to
b2568f0
Compare
robbavey
requested changes
Dec 15, 2022
robbavey
left a comment
robbavey
left a comment
Member
There was a problem hiding this comment.
Code looks fine, but there is an incomplete looking unit test
| expect { subject.register }.to_not raise_error | ||
| end | ||
|
|
||
| context "x" do |
Member
There was a problem hiding this comment.
This test looks unfinished - can you fix this up before committing
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Release notes
Update kafka client to 3.3.1, requires Logstash >= 8.3.0. Deprecated
defaultvalue for settingclient_dns_lookup, when used it's forced touse_all_dns_ipsWhat does this PR do?
Updated the Kafka client library used from
2.8.1to3.3.1and Schema Registry from6.2.2to7.3.0.This PR updates the client library and, due to the removal of
zookeeperflag in launching integration Kafka instance, update also the bash scripts used in integration testing.The update of the Schema Registry forces the dependency on Logstash 8.3 because it's the first that ships Jackson
2.13.3which is strictly required during the instantiation of the client.Why is it important/What is the impact to the user?
Updating the client requires a library (Jackson) that's available from Logstash
8.3.0so this is a requirement starting from this version of the plugin.The new Kafka client dropped the
defaultvalue for configurationclient_dns_lookup. Starting from Kafka2.6.0it had been deprecated and was removed in Kafka3.0.The default value for plugin setting
client_dns_lookupswitched fromdefaulttouse_all_dns_ipswhich if a user doesn't customize i,t doesn't create any problem.If a user explicitly set
client_dns_lookuptodefaultand updates the plugin to this version then the value is forced touse_all_dns_ips.Simple explanation why
defaultis not a good choice, see here.Checklist
[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
curl "https://archive.apache.org/dist/kafka/3.3.1/kafka_2.12-3.3.1.tgz" -o kafka_2.12-3.3.1.tgzcd-ing into the uncompressed folderlogstash_integration_topic_plainto be later used into the Logstash pipelineGemfileThe events has to be received and logged by Logstash.
Shutdown and cleanup:
bin/kafka-topics.sh --bootstrap-server localhost:9092 --delete --topic 'logstash_.*' bin/kafka-server-stop.sh bin/zookeeper-server-stop.shRelated issues
Use cases
As a Kafka user I want to be able to use a Logstash plugin aligned to the latest major version of Kafka.
Screenshots
Logs