-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add support for using CooperativeStickyAssignor with KafkaConsumer #844
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import cats.effect.unsafe.implicits.global | |
import fs2.Stream | ||
import fs2.concurrent.SignallingRef | ||
import fs2.kafka.internal.converters.collection._ | ||
import org.apache.kafka.clients.consumer.NoOffsetForPartitionException | ||
import org.apache.kafka.clients.consumer.{ConsumerConfig, CooperativeStickyAssignor, NoOffsetForPartitionException} | ||
import org.apache.kafka.common.TopicPartition | ||
import org.apache.kafka.common.errors.TimeoutException | ||
import org.scalatest.Assertion | ||
|
@@ -516,6 +516,7 @@ final class KafkaConsumerSpec extends BaseKafkaSpec { | |
} | ||
} | ||
.takeWhile(_.size < 200) | ||
.timeout(20.seconds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timeout added to match test with custom assignor. This test was working without it, while the one for the custom assignor would hang indefinitely. |
||
.compile | ||
.drain | ||
.guarantee(stopSignal.set(true)) | ||
|
@@ -540,6 +541,88 @@ final class KafkaConsumerSpec extends BaseKafkaSpec { | |
} | ||
} | ||
|
||
it("should handle rebalance with CooperativeStickyAssignor") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a copy of the |
||
withTopic { topic => | ||
createCustomTopic(topic, partitions = 3) | ||
val produced1 = (0 until 100).map(n => s"key-$n" -> s"value->$n") | ||
val produced2 = (100 until 200).map(n => s"key-$n" -> s"value->$n") | ||
val producedTotal = produced1.size.toLong + produced2.size.toLong | ||
|
||
def startConsumer( | ||
consumedQueue: Queue[IO, CommittableConsumerRecord[IO, String, String]], | ||
stopSignal: SignallingRef[IO, Boolean] | ||
): IO[Fiber[IO, Throwable, Vector[Set[Int]]]] = | ||
Ref[IO] | ||
.of(Vector.empty[Set[Int]]) | ||
.flatMap { assignedPartitionsRef => | ||
KafkaConsumer | ||
.stream( | ||
consumerSettings[IO] | ||
.withProperties( | ||
ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG -> classOf[CooperativeStickyAssignor].getName | ||
) | ||
) | ||
.subscribeTo(topic) | ||
.flatMap(_.partitionsMapStream) | ||
.filter(_.nonEmpty) | ||
.evalMap { assignment => | ||
assignedPartitionsRef.update(_ :+ assignment.keySet.map(_.partition())).as { | ||
Stream | ||
.emits(assignment.map { | ||
case (_, stream) => | ||
stream.evalMap(consumedQueue.offer) | ||
}.toList) | ||
.covary[IO] | ||
} | ||
} | ||
.flatten | ||
.parJoinUnbounded | ||
.interruptWhen(stopSignal) | ||
.compile | ||
.drain >> assignedPartitionsRef.get | ||
} | ||
.start | ||
|
||
(for { | ||
stopSignal <- SignallingRef[IO, Boolean](false) | ||
queue <- Queue.unbounded[IO, CommittableConsumerRecord[IO, String, String]] | ||
ref <- Ref.of[IO, Map[String, Int]](Map.empty) | ||
fiber1 <- startConsumer(queue, stopSignal) | ||
_ <- IO.sleep(5.seconds) | ||
_ <- IO(publishToKafka(topic, produced1)) | ||
fiber2 <- startConsumer(queue, stopSignal) | ||
_ <- IO.sleep(5.seconds) | ||
_ <- IO(publishToKafka(topic, produced2)) | ||
_ <- Stream | ||
.fromQueueUnterminated(queue) | ||
.evalMap { committable => | ||
ref.modify { counts => | ||
val key = committable.record.key | ||
val newCounts = counts.updated(key, counts.getOrElse(key, 0) + 1) | ||
(newCounts, newCounts) | ||
} | ||
} | ||
.takeWhile(_.size < 200) | ||
.timeout(20.seconds) | ||
.compile | ||
.drain | ||
.guarantee(stopSignal.set(true)) | ||
consumer1assignments <- fiber1.joinWithNever | ||
consumer2assignments <- fiber2.joinWithNever | ||
keys <- ref.get | ||
} yield { | ||
assert { | ||
keys.size.toLong == producedTotal && | ||
keys.values.sum == 236 && | ||
consumer1assignments.size == 1 && | ||
consumer1assignments(0) == Set(0, 1, 2) && | ||
consumer2assignments.size == 1 && | ||
consumer2assignments(0) == Set(2) | ||
} | ||
}).unsafeRunSync() | ||
} | ||
} | ||
|
||
it("should close all old streams on rebalance") { | ||
withTopic { topic => | ||
val numPartitions = 3 | ||
|
@@ -698,7 +781,7 @@ final class KafkaConsumerSpec extends BaseKafkaSpec { | |
consumer1Updates(2).isEmpty && | ||
consumer1Updates(3).size < 3 && | ||
// Startup assignments (zero), initial assignments (< 3) | ||
consumer2Updates.length == 2 | ||
consumer2Updates.length == 2 && | ||
Comment on lines
-701
to
+784
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this affected |
||
consumer2Updates.head.isEmpty && | ||
consumer2Updates(1).size < 3 && | ||
(consumer1Updates(3) ++ consumer2Updates(1)) == consumer1Updates(1) | ||
|
@@ -707,6 +790,65 @@ final class KafkaConsumerSpec extends BaseKafkaSpec { | |
} | ||
} | ||
|
||
it("should stream assignment updates to listeners when using CooperativeStickyAssignor") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a copy of the existing |
||
withTopic { topic => | ||
createCustomTopic(topic, partitions = 3) | ||
|
||
val consumer = | ||
for { | ||
queue <- Stream.eval(Queue.unbounded[IO, Option[SortedSet[TopicPartition]]]) | ||
_ <- KafkaConsumer | ||
.stream( | ||
consumerSettings[IO] | ||
.withProperties( | ||
ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG -> classOf[CooperativeStickyAssignor].getName | ||
) | ||
|
||
) | ||
.subscribeTo(topic) | ||
.evalMap { consumer => | ||
consumer.assignmentStream | ||
.concurrently(consumer.records) | ||
.evalMap(as => queue.offer(Some(as))) | ||
.compile | ||
.drain | ||
.start | ||
.void | ||
} | ||
} yield { | ||
queue | ||
} | ||
|
||
(for { | ||
queue1 <- consumer | ||
_ <- Stream.eval(IO.sleep(5.seconds)) | ||
queue2 <- consumer | ||
_ <- Stream.eval(IO.sleep(5.seconds)) | ||
_ <- Stream.eval(queue1.offer(None)) | ||
_ <- Stream.eval(queue2.offer(None)) | ||
consumer1Updates <- Stream.eval( | ||
Stream.fromQueueNoneTerminated(queue1).compile.toList | ||
) | ||
consumer2Updates <- Stream.eval( | ||
Stream.fromQueueNoneTerminated(queue2).compile.toList | ||
) | ||
_ <- Stream.eval(IO(assert { | ||
// Startup assignment (zero), initial assignment (all partitions), | ||
// minimal revocation when 2nd joins (keep two) | ||
consumer1Updates.length == 3 && | ||
consumer1Updates.head.isEmpty && | ||
consumer1Updates(1).size == 3 && | ||
consumer1Updates(2).size == 2 && | ||
// Startup assignments (zero), initial assignments (one) | ||
consumer2Updates.length == 2 && | ||
consumer2Updates.head.isEmpty && | ||
consumer2Updates(1).size == 1 && | ||
(consumer1Updates(2) ++ consumer2Updates(1)) == consumer1Updates(1) | ||
})) | ||
} yield ()).compile.drain.unsafeRunSync() | ||
} | ||
} | ||
|
||
it("begin from the current assignments") { | ||
withTopic { topic => | ||
createCustomTopic(topic, partitions = 3) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would appreciate a comment on this change in particular.
The change makes sense to me, logically:
revokedFetches
andrevokedNonFetches
add up torevoked
. But I did not dig enough into the implications to be sure this is the intention. For instance: in which combination of pending records and pending fetches it will make a difference, and what is the consequence?I thought I had left this out of the PR, but while it's here we might as well discuss it.