Use confluent-kafka-python for Kafka admin client instead of kafka-python #757
Conversation
This file contains 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
matyaskuti
force-pushed
the
matyaskuti/confluent_kafka_for_admin_client
branch
6 times, most recently
from
82962fe
to
8a91b03
Compare
matyaskuti
changed the title
The new method is a confluent-kafka-python compatible callback, which will be required by the new Kafka admin client. At the same time, remove the token from the token provider classes' `repr` - while we don't log these, it's better not to accidentally leak tokens.
matyaskuti
force-pushed
the
matyaskuti/confluent_kafka_for_admin_client
branch
from
8a91b03
to
80bb504
Compare
eliax1996
reviewed
Nov 13, 2023
There was a problem hiding this comment.
Very nice code 😄, I've left some comments, feel free to disagree with all of them.
I've just left what I was feeling while reading the code. The code looks very auto explicative and you've added doc on all the new methods cool!
PPS before merging just make sure to sync the well doc you added to the commits with the pr description.
I need another round of reading but I've left this to flush my mental buffer :)
matyaskuti
force-pushed
the
matyaskuti/confluent_kafka_for_admin_client
branch
2 times, most recently
from
119860b
to
485fbb8
Compare
eliax1996
reviewed
Nov 14, 2023
aiven-anton
reviewed
Nov 15, 2023
matyaskuti
force-pushed
the
matyaskuti/confluent_kafka_for_admin_client
branch
4 times, most recently
from
7293345
to
cfdc71a
Compare
This change replaces all Kafka admin client usages with a new implementation, based on confluent-kafka-python's `AdminClient`, aiming to keep the same interface as much as possible for the time being. The goal is not to completely remove kafka-python as a dependency, rather take a step towards that. The kafka-python library is still widely used in the codebase, eg. for error handling (even in the new `KafkaAdminClient`). The new `KafkaAdminClient` takes the place of all previous admin clients used, thus not specific to the REST proxy anymore. Hence it's move to a new module as `karapace.kafka_admin.KafkaAdminClient`, which also consolidates all references to the `confluent_kafka` lib. The test suite has also been moved and expanded. Wherever possible the same interface was kept, only adding new methods to simplify client code (to a minimal extent). This is most notable in tests and the backups functionality. The new admin client aims to hide away confluent-kafka-python's Future-based async client approach by providing wrapper methods that resolve these futures. Typing (issues) from `confluent_kafka` are ignored for now, no typing stubs have been added on purpose. Resources: * confluent-kafka-python documentation: https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html# * librdkafka configuration documentation: https://github.com/confluentinc/librdkafka/blob/master/CONFIGURATION.md
matyaskuti
force-pushed
the
matyaskuti/confluent_kafka_for_admin_client
branch
2 times, most recently
from
1038d9b
to
79c6198
Compare
aiven-anton
reviewed
Nov 20, 2023
| @@ -14,6 +14,7 @@ warn_unused_ignores = True | |||
| warn_no_return = True | |||
| warn_unreachable = True | |||
| strict_equality = True | |||
| enable_incomplete_feature = Unpack | |||
There was a problem hiding this comment.
I noticed this was just marked stable in 1.7 that was just released, oddly coincidental timing! https://mypy-lang.blogspot.com/2023/11/mypy-17-released.html
But let's keep this as-is and upgrade mypy separately to limit the scope.
aiven-anton
approved these changes
Nov 20, 2023
|
This looks good to me 👍 @eliax1996 Do you want to make another pass on this? |
eliax1996
approved these changes
Nov 20, 2023
This was referenced Nov 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
About this change - What it does
Replace Kafka admin clients (including
KafkaRestAdminClientand "vanilla" admin clients fromkafka-python) with a newconfluent_kafkabased one.The main change is:
KafkaAdminClientin a new moduleKafkaRestAdminClientfrom its original location and adapting to the new base class from confluent-kafka-pythonOther changes are mostly a consequence of this, adapting to a slightly different interface, simplifying client code when it was straight forward, etc. Many files have been changed, however, most of these are straightforward renamings or small changes to adapt to the new client.
Why this way
kafka-pythonis out of scope for this PR, however there is only one admin client nowKafkaAdminClientaims to keep the same interface as much as possible as it was before (exceptions include many-manyNewTopic.name->NewTopic.topicchanges), also in exceptions thrownconfluent_kafkawithin the new modulekafka.errors.for_codeand its specific exceptionsNo typing stubs are added for confluent-kafka-python, that's something that could potentially be done after a "full switch"Type stubs are going to be added in a follow-up: Add typing stubs for confluent-kafka #760Request from reviewers
Are there any "what if" kind of scenarios that might not be covered by tests or the code at the moment? (Especially backups and schema registry related.)