search

dashbitco / broadway_kafka

A Broadway connector for Kafka
233 stars 53 forks source link

fix: check draining state from ets #123

Closed Goose97 closed 1 year ago

Goose97 commented 1 year ago

This pull implements a fix for pull https://github.com/dashbitco/broadway_kafka/pull/121

To check if we're in draining state or not, we should check both from state and from ets table

This fixes https://github.com/dashbitco/broadway_kafka/issues/118

josevalim commented 1 year ago

/cc @slashmili

slashmili commented 1 year ago

@Goose97, Seems correct. Sorry I forgot about that. Checking draining_after_revoke_flag is the way!

However I think checking revoke_caller is redundant as both the flag and revoke_caller set to false and nil at the same place.

https://github.com/dashbitco/broadway_kafka/blob/31aeffd4d3e3a78640603f2d463f588a9159665e/lib/broadway_kafka/producer.ex#L418-L420

josevalim commented 1 year ago

Can you please check now @slashmili?

slashmili commented 1 year ago

LGTM 👍🏼

josevalim commented 1 year ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart:

Goose97 commented 1 year ago

@slashmili Well, that was my initial decision. But then I decide to check from local state first to save one ETS call.