Skip to content
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.

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

Replace sync Kafka consumers with confluent_kafka one #794

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

matyaskuti
Copy link
Contributor

@matyaskuti matyaskuti commented Jan 10, 2024

About this change - What it does

Why this way

  • Using consumer.assign after consumer.subscribe is not supposed to be done (further tests and guards will be introduced in a later PR which adds enriched functionality to consumers)

@matyaskuti matyaskuti force-pushed the matyaskuti/confluent_kafka_consumers branch 2 times, most recently from 480822a to 80f07dc Compare January 10, 2024 14:43
@matyaskuti matyaskuti marked this pull request as ready for review January 11, 2024 08:26
@matyaskuti matyaskuti requested review from a team as code owners January 11, 2024 08:26
@jjaakola-aiven
Copy link
Contributor

@matyaskuti Could you comment the exact spot in the PR for the spot where the bug was?

@matyaskuti
Copy link
Contributor Author

@jjaakola-aiven I updated the description with the location. After a short discussion with @aiven-anton I'll attempt to add a test that reveals this issue through the backups code as well.

@matyaskuti matyaskuti force-pushed the matyaskuti/confluent_kafka_consumers branch from 80f07dc to c4549d0 Compare January 11, 2024 10:42
tests/integration/backup/test_v3_backup.py Outdated Show resolved Hide resolved
tests/integration/kafka/test_consumer.py Outdated Show resolved Hide resolved
tests/integration/backup/test_v3_backup.py Outdated Show resolved Hide resolved
@matyaskuti matyaskuti force-pushed the matyaskuti/confluent_kafka_consumers branch from c4549d0 to a30608f Compare January 11, 2024 12:41
When instantiating the consumer we already a subscribe to the topic that
is to be backed up (ie. __schemas). Using assign on top of that causes
problems - once the assignment actually takes effect, the consumer
starts to consume from the beginning, ultimately resulting in an
`InconsistentOffset` error.

The default subscribe is enough at the moment, as we do not support
backing up a multi-partition topic.
@matyaskuti matyaskuti force-pushed the matyaskuti/confluent_kafka_consumers branch from a30608f to 2c03ba0 Compare January 11, 2024 12:48
@matyaskuti matyaskuti dismissed aiven-anton’s stale review January 11, 2024 13:16

Comments addressed

Copy link
Contributor

@aiven-anton aiven-anton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Rather than review the whole diff, I verified that 7fde3188b470528801c983b43592cf6ac182ff19 is the exact same as the originally merged commit, and then reviewed 2c03ba0a4e881fb58665461258fe184785279ddd independently.

@aiven-anton aiven-anton merged commit 59eaff2 into main Jan 12, 2024
8 checks passed
@aiven-anton aiven-anton deleted the matyaskuti/confluent_kafka_consumers branch January 12, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants