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

CommittableOffsetBatch: Handle multiple committer refs #953

Merged
merged 6 commits into from
Oct 25, 2019

Conversation

ennru
Copy link
Member

@ennru ennru commented Oct 25, 2019

Purpose

Harden the CommittableOffsetBatch so it can aggregate offsets with committer refs from several consumer stages/actors.

References

Problem reported in #942

Changes

(Reviewing per commit might make sense)

  • Test various scenarios where different consumer stages/actors feed a commit batch
  • Fix equals in KafkaAsyncConsumerCommitterRef
  • Keep committer refs per GroupId-Topic-Partition instead of just per group and send per offsets per that combination to the committing actor
  • Store the to-be-committed offsets as tuple lists in the actor

Background Context

The CommittableOffsetBatch guarded against batching offsets for the same group ID that came from different consumer stages/actors. That becomes a problem for unusual flow topologies, and when a RestartSource is used.

@ennru ennru added this to the 2.0.0 milestone Oct 25, 2019
@seglo seglo self-requested a review October 25, 2019 13:50
Copy link
Contributor

@seglo seglo left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work, I like that this impl actually cleans up the batch updating logic.

*
* This should be case class to be comparable based on consumerActor and commitTimeout. This comparison is used in [[CommittableOffsetBatchImpl]].
* Sends [[akka.kafka.internal.KafkaConsumerActor.Internal.Commit]] and
* [[akka.kafka.internal.KafkaConsumerActor.Internal.CommitSingle]] messages to the consumer actor.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add CommitWithoutReply too for completeness.

@ennru ennru merged commit 1da2090 into akka:master Oct 25, 2019
@ennru ennru deleted the commitbatch branch October 25, 2019 18:28
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.

2 participants