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

SKYEDEN-3020 | consumers retransmission improvements #1941

Merged
merged 11 commits into from
Jan 10, 2025

Conversation

MarcinBobinski
Copy link
Collaborator

@MarcinBobinski MarcinBobinski commented Dec 23, 2024

In the current retransmission process, offsets are updated only locally in Hermes consumers via KafkaConsumer::seek().

This pull request introduces a change to also commit offsets to Kafka using KafkaConsumer::commitSync().

Furthermore, it increments the partitionAssignmentTerm to ensure Kafka-committed offsets are not overwritten by concurrently processed events created before retransmission.

@MarcinBobinski MarcinBobinski force-pushed the SKYEDEN-3020-ConsumerRetransmissionImprovements branch from 9981d34 to b304b45 Compare December 23, 2024 11:07
@MarcinBobinski MarcinBobinski changed the title SKYEDEN-3271 | consumers retransmission improvements SKYEDEN-3020 | consumers retransmission improvements Dec 23, 2024
MarcinBobinski and others added 3 commits January 8, 2025 11:23
…erRetransmissionImprovements

# Conflicts:
#	hermes-consumers/src/main/java/pl/allegro/tech/hermes/consumers/consumer/receiver/MessageReceiver.java
if (!movedOffsets.isEmpty()) {
// Incrementing assignment term ensures that currently committed offsets won't be overwritten
// by the events from the past which are concurrently processed by the consumer
partitionAssignmentState.incrementTerm(subscriptionName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are You sure that this is the correct order to commit first and then increment assignment term?

Copy link
Collaborator Author

@MarcinBobinski MarcinBobinski Jan 9, 2025

Choose a reason for hiding this comment

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

Order doesn't matter in this case. Even though the events are processed concurrently, the commitment of offsets is synchronous. The method commitIfReady can only be executed when the entire process of retransmission is complete.

szczygiel-m
szczygiel-m previously approved these changes Jan 9, 2025
moscicky
moscicky previously approved these changes Jan 9, 2025
…erRetransmissionImprovements

# Conflicts:
#	integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/KafkaRetransmissionServiceTest.java
@MarcinBobinski MarcinBobinski dismissed stale reviews from moscicky and szczygiel-m via 4e2865e January 10, 2025 08:53
@szczygiel-m szczygiel-m merged commit 80ab55c into master Jan 10, 2025
13 checks passed
@szczygiel-m szczygiel-m deleted the SKYEDEN-3020-ConsumerRetransmissionImprovements branch January 10, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants