confluentinc / parallel-consumer

Parallel Apache Kafka client wrapper with per message ACK, client side queueing, a simpler consumer/producer API with key concurrency and extendable non-blocking IO processing.
https://confluent.io/confluent-accelerators/#parallel-consumer
Apache License 2.0
76 stars 125 forks source link

stale containers exclusion and handling improvement #779

Closed sangreal closed 3 months ago

sangreal commented 4 months ago

Description... in previous logic we need two passes to get the normal containers and put stale containers into mailbox queue. But now we only need one pass, therefore when the traffic is huge, there will be performance gain.

Checklist

rkolesnev commented 3 months ago

Hey @sangreal - is the added complexity worth the improvement? My understanding that performance improvement will only apply when there is stale work due to rebalance / epoch change - but such an event should really be rare relatively speaking - so few millisecond performance improvement on one-off scans wouldn't warrant additional changes in my opinion. What do you think?

sangreal commented 3 months ago

@rkolesnev Thanks for the reply.

  1. I think the probability is not that low since when I encountered the staled containers causing poller paused last year, the assigned to the same partition causing stale containers happened a time a few days using cooperativestickyassignor. Therefore the fix could be valuable.
  2. the new logic is quite straightforward and with UTs, I don't think this is quite complex.

Happy to hear your opinion on these.

rkolesnev commented 3 months ago

/sem-approve

rkolesnev commented 3 months ago

Overall logic looks solid - i've suggested to move the filtering earlier in the flow to eliminate re-scanning and make it a bit more clear upfront.

rkolesnev commented 3 months ago

And if you can please run license:format or update license headers in the changed test files to 2024

sangreal commented 3 months ago

And if you can please run license:format or update license headers in the changed test files to 2024

Sure, headers have been updated

rkolesnev commented 3 months ago

/sem-approve