Skip to content

Additional consumer functionality exposed #919

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

Merged
merged 9 commits into from
Apr 1, 2022

Conversation

dmedser
Copy link
Contributor

@dmedser dmedser commented Mar 27, 2022

New methods added to trait KafkaOffsets[F[_]]:

def committed(partitions: Set[TopicPartition]): F[Map[TopicPartition, OffsetAndMetadata]]

def committed(partitions: Set[TopicPartition], timeout: FiniteDuration): F[Map[TopicPartition, OffsetAndMetadata]]

@dmedser
Copy link
Contributor Author

dmedser commented Mar 27, 2022

@bplommer @LMnet could you please give me a hint how to fix this CI error:

* abstract method committed(scala.collection.immutable.Set)java.lang.Object in interface fs2.kafka.consumer.KafkaOffsets is present only in current version?

Can I just add

ProblemFilters.exclude[ReversedMissingMethodProblem]("fs2.kafka.consumer.KafkaOffsets.committed")

to mimaBinaryIssueFilters?

@LMnet
Copy link
Member

LMnet commented Mar 28, 2022

Hi @dmedser

Yes, you have to change MIMA rules to fix CI error.

…dProblem]("fs2.kafka.consumer.KafkaOffsets.committed")
@bplommer
Copy link
Member

This is going to pass CI, but the change is genuinely going to break binary compatibility. So the options are either to retarget this PR at Series/3.x or add a new trait (KafkaOffsetsV2?) that extends KafkaOffsets.

@dmedser
Copy link
Contributor Author

dmedser commented Mar 28, 2022

This is going to pass CI, but the change is genuinely going to break binary compatibility. So the options are either to retarget this PR at Series/3.x or add a new trait (KafkaOffsetsV2?) that extends KafkaOffsets.

@bplommer Does this mean that whenever someone wants to add some additional functionality, they will have to add a new trait with name KafkaXVN where X is Offsets and N is 2 as in my case?

Also if we consider this case with additional trait, what should KafkaConsumer look like? I suppose something like this:

sealed abstract class KafkaConsumer[F[_], K, V]
  extends KafkaConsume[F, K, V]
    with . . .
    with KafkaOffsetsV2[F]
    with . . .

where KafkaOffsetsV2 extends KafkaOffsets, right?

@LMnet
Copy link
Member

LMnet commented Mar 28, 2022

change is genuinely going to break binary compatibility

Are you sure? As far as I know, adding new methods will not violate binary compatibility.

dmedser added 2 commits March 28, 2022 13:53
…dd-committed-to-kafka-offsets

� Conflicts:
�	modules/core/src/main/scala/fs2/kafka/KafkaConsumer.scala
@bplommer
Copy link
Member

Are you sure? As far as I know, adding new methods will not violate binary compatibility.

Yes, because it's an unsealed trait. A mock KafkaOffsets implementation compiled against a previous version won't have implementations, but a user of that mock compiled against the new version will expect them, leading to a possible NoSuchMethodException.

@bplommer
Copy link
Member

@bplommer Does this mean that whenever someone wants to add some additional functionality, they will have to add a new trait with name KafkaXVN where X is Offsets and N is 2 as in my case?

Also if we consider this case with additional trait, what should KafkaConsumer look like? I suppose something like this:

sealed abstract class KafkaConsumer[F[_], K, V]
  extends KafkaConsume[F, K, V]
    with . . .
    with KafkaOffsetsV2[F]
    with . . .

where KafkaOffsetsV2 extends KafkaOffsets, right?

Yes, that's all correct.

@dmedser
Copy link
Contributor Author

dmedser commented Mar 28, 2022

@bplommer Is everything correct this time?

@dmedser
Copy link
Contributor Author

dmedser commented Mar 31, 2022

@bplommer @LMnet could you guys trigger pipeline again please?

Copy link
Member

@bplommer bplommer left a comment

Choose a reason for hiding this comment

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

LGTM

@LMnet LMnet merged commit d93aa4c into fd4s:series/2.x Apr 1, 2022
@dmedser dmedser deleted the add-committed-to-kafka-offsets branch April 1, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants