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

[POC - 3] New commit interface #1042

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

guizmaii
Copy link
Member

@guizmaii guizmaii commented Sep 9, 2023

No description provided.

@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch from 0fb7fcb to a2f446e Compare September 9, 2023 18:09
.flatMap(offsetBatch =>
commitTransactionWithOffsets(
offsetBatch,
consumerGroupMetadata = None // TODO Jules
Copy link
Member Author

@guizmaii guizmaii Sep 9, 2023

Choose a reason for hiding this comment

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

TODO @guizmaii: How to pass this information here?

@@ -219,23 +214,16 @@ private[consumer] final class Runloop private (
if (streams.isEmpty) ZIO.succeed(fulfillResult)
else {
for {
consumerGroupMetadata <- getConsumerGroupMetadataIfAny
/*consumerGroupMetadata*/ _ <- getConsumerGroupMetadataIfAny // TODO Jules: Use: how?
Copy link
Member Author

@guizmaii guizmaii Sep 9, 2023

Choose a reason for hiding this comment

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

TODO @guizmaii: Find how to pass this information to the transactional producer

@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch 2 times, most recently from da3ed50 to b7bc8d7 Compare September 9, 2023 18:31
}
.transduce(Consumer.offsetBatches)
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO @guizmaii: What was .tranduce doing?

@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch from b7bc8d7 to b1ade41 Compare September 9, 2023 18:49
Comment on lines +11 to +34
@inline
private[zio] def topicPartition(record: ConsumerRecord[_, _]): TopicPartition =
new TopicPartition(record.topic(), record.partition())

private[zio] def deserializeWith[R, K, V](
keyDeserializer: Deserializer[R, K],
valueDeserializer: Deserializer[R, V]
)(record: ConsumerRecord[Array[Byte], Array[Byte]]): RIO[R, ConsumerRecord[K, V]] =
for {
key <- keyDeserializer.deserialize(record.topic(), record.headers(), record.key())
value <- valueDeserializer.deserialize(record.topic(), record.headers(), record.value())
} yield new ConsumerRecord[K, V](
record.topic(),
record.partition(),
record.offset(),
record.timestamp(),
record.timestampType(),
record.serializedKeySize(),
record.serializedValueSize(),
key,
value,
record.headers(),
record.leaderEpoch()
)
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO @guizmaii: Move?

@guizmaii guizmaii changed the base branch from comsumer_commit_records_awaitable to master September 9, 2023 18:58
@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch from 1ae11f0 to ad7fb69 Compare September 9, 2023 19:04
@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch from fc9b7e9 to 2b88dce Compare September 9, 2023 19:22
@guizmaii guizmaii force-pushed the comsumer_commit_records_cleaned branch from 2b88dce to a60db8e Compare September 9, 2023 19:25
@@ -30,11 +31,11 @@ private[consumer] final class RunloopAccess private (
diagnostics: Diagnostics
) {

private def withRunloopZIO[E](
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO @guizmaii: Put back previous signature

@@ -37,6 +38,14 @@ trait Consumer {
timeout: Duration = Duration.Infinity
): Task[Map[TopicPartition, Long]]

def commit(record: ConsumerRecord[_, _]): Task[Unit]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is 6 extra methods. I propose we commit only Offset like objects, not entire ConsumerRecords. Semantically that is better anyway, to 'commit an offset' instead of 'commit a record', the latter suggesting you can ack single records.

It would be nice to have a supertype for the Offset and the OffsetBatch, maybe CommittableOffset. Alpakka has Committable for the same concept.

@svroonland
Copy link
Collaborator

@guizmaii Okay with you if we close this PR, being over a year old? I'd still like to explore a Consumer.commit(offset) or something similar, we can create an issue for follow-up.

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.

2 participants