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

Can alter offsets on RebalanceBeforeAssign. #180

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Kafka/Consumer.hs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ newConsumer :: MonadIO m
-> m (Either KafkaError KafkaConsumer)
newConsumer props (Subscription ts tp) = liftIO $ do
let cp = case cpCallbackPollMode props of
CallbackPollModeAsync -> setCallback (rebalanceCallback (\_ _ -> return ())) <> props
CallbackPollModeAsync -> setCallback (rebalanceCallback (\_ _ -> return Nothing)) <> props
CallbackPollModeSync -> props
kc@(KafkaConf kc' qref _) <- newConsumerConf cp
tp' <- topicConf (TopicProps tp)
Expand Down
29 changes: 19 additions & 10 deletions src/Kafka/Consumer/Callbacks.hs
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,20 @@ import Control.Monad (forM_, void)
import Foreign.ForeignPtr (newForeignPtr_)
import Foreign.Ptr (nullPtr)
import Kafka.Callbacks as X
import Kafka.Consumer.Convert (fromNativeTopicPartitionList', fromNativeTopicPartitionList'')
import Kafka.Consumer.Convert (fromNativeTopicPartitionList', fromNativeTopicPartitionList'', toNativeTopicPartitionList)
import Kafka.Consumer.Types (KafkaConsumer (..), RebalanceEvent (..), TopicPartition (..))
import Kafka.Internal.RdKafka
import Kafka.Internal.Setup (HasKafka (..), HasKafkaConf (..), Kafka (..), KafkaConf (..), getRdMsgQueue, Callback (..))
import Kafka.Types (KafkaError (..), PartitionId (..), TopicName (..))

import qualified Data.Text as Text

-- | Sets a callback that is called when rebalance is needed.
rebalanceCallback :: (KafkaConsumer -> RebalanceEvent -> IO ()) -> Callback
-- | Sets a callback that is called when rebalance is happening.
--
-- If you want to store the offsets locally, return the TopicPartition list with
-- modified offsets from RebalanceBeforeAssign. Callback return value on other
-- events is ignored.
rebalanceCallback :: (KafkaConsumer -> RebalanceEvent -> IO (Maybe [TopicPartition])) -> Callback
rebalanceCallback callback =
Callback $ \kc@(KafkaConf con _ _) -> rdKafkaConfSetRebalanceCb con (realCb kc)
where
Expand Down Expand Up @@ -54,19 +58,24 @@ redirectPartitionQueue (Kafka k) (TopicName t) (PartitionId p) q = do
Nothing -> return ()
Just pq -> rdKafkaQueueForward pq q

setRebalanceCallback :: (KafkaConsumer -> RebalanceEvent -> IO ())
setRebalanceCallback :: (KafkaConsumer -> RebalanceEvent -> IO (Maybe [TopicPartition]))
-> KafkaConsumer
-> KafkaError
-> RdKafkaTopicPartitionListTPtr -> IO ()
setRebalanceCallback f k e pls = do
ps <- fromNativeTopicPartitionList'' pls
let assignment = (tpTopicName &&& tpPartition) <$> ps
let (Kafka kptr) = getKafka k

case e of
KafkaResponseError RdKafkaRespErrAssignPartitions -> do
f k (RebalanceBeforeAssign assignment)
void $ rdKafkaAssign kptr pls
-- Consumer may want to alter the offsets if they are stored locally.
mbAltered <- f k (RebalanceBeforeAssign ps)
(pls', assigned) <- case mbAltered of
Nothing -> pure (pls, ps)
Just alt -> do
pls' <- toNativeTopicPartitionList alt
pure (pls', alt)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some code to guarantee that all (and only) the partitions from the "original" assignment are passed to rdKafkaAssign?

Currently there is a possibility for me to return Just a subset (or a superset) of the partitions. Which may cause problems.
But if we take a list of altered partitions, add the "missing" ones and remove "extra" ones then it should be fine?

Copy link
Member

Choose a reason for hiding this comment

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

@robinp ping ;)

Copy link
Author

Choose a reason for hiding this comment

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

Hey, sorry for the late reply. How about returning IO (Maybe (TopicPartition -> Maybe PartitionOffset)), where an inner Nothing result would mean "don't change offset". This leaves no questions what can and can't be done.

If this sounds right for you and have the capacity, I don't mind you adding to the PR. Otherwise feel free to defer to me, just expect slight delay. Thanks for looking at this!

Copy link
Member

Choose a reason for hiding this comment

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

@robinp Hmm... From one side - yes... From the other side, do we know that we can demand a pure function here?
Perhaps IO (Maybe (TopicPartition -> IO (Maybe PartitionOffset))), but it gets clunky...

In my mind the original semantics IO (Maybe [TopicPartition]) would read as "give me all you know and I'll do what I can". I.E. I could always return all the offsets of for all the partitions (for the whole topic) if I wanted, but the internals of the callback would figure out the applicable ones for a given consumer in a group...

For that I would even suggest simplifying it to IO [TopicPartition] and say that "we will use offsets that you provide for the partitions that we actually assign" and that'd be it?

Copy link
Author

Choose a reason for hiding this comment

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

IO-in-IO is not really needed, since the callback already runs in IO, it can query whatever it needs in IO, create the pure structures, and return the offset function based on those pure structures. That practically says, by the time the callback returns, it made up its mind about the offset decisions it would take.

Returning a [TopicPartition] would be deceiving, since suggests it would be used as-is, but as you say, we would need to make adjustments to it to fulfill the Kafka contract.

If we want to cut down on the typesig a bit, then IO (TopicPartition -> Maybe PartitionOffset) can work, as one can always return const Nothing, but an alias like dontOverrideOffsets can be added as well.

Copy link
Member

Choose a reason for hiding this comment

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

True, we can compute all the offsets and then construct a pure function and return it... That won't even need the first Maybe in this case, IO (TopicPartition -> Maybe PartitionOffset) would be sufficient...

Or perhaps even IO (TopicPartition -> PartitionOffset) since we can always project the original offset into the new one... But I see how this redundant Maybe can be appealing here...

Well, I myself would be happy with just IO [TopicPartition] + merging, since it would be (to me) the most convenient for usage at the price of one bit of knowledge: the returned list cannot alter the list of partitions that are going to be assigned.

But I am not against IO (TopicPartition -> Maybe PartitionOffset) or IO (TopicPartition -> PartitionOffset), too.

One thing that we could do is to keep setRebalanceCallback signature the way it was before, and introduce setRebalanceCallbackWithOffsets with the new signature.
setRebalanceCallback then could just delegate to the new function with a function that doesn't change offsets.
This will allow us to avoid introducing a breaking change.

Do you agree?

Copy link
Author

Choose a reason for hiding this comment

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

That transition path for smooth upgrade sounds nice.

Yeah, I like IO (TopicPartition -> Maybe PartitionOffset) because I can just const Nothing without having to do a mental exercise of thinking "Can I always return latest? Or unassigned? Or gather the specific value that is available?". I can trust Nothing to do the right thing, hopefully.

Could drop that Maybe, but then some clear guidance is needed in the function doc which value should be returned. Wondering if there's "one true way", in which case rather implement that + have the Maybe, or if there's value in maintaining freedom about multiple behaviors.

Copy link
Member

Choose a reason for hiding this comment

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

Let's settle on something like

rebalanceCallbackWithOffsets :: (KafkaConsumer -> RebalanceEvent -> IO (TopicPartition -> Maybe PartitionOffset)) -> Callback
rebalanceCallbackWithOffsets = <new implementation>

rebalanceCallback :: (KafkaConsumer -> RebalanceEvent -> IO ()) -> Callback
rebalanceCallback f = <call rebalanceCallbackWithOffsets providing (const Nothing)>

I guess that it'll be close enough to the best of both having this feature and not breaking compatibility for consumers who don't care about the offsets.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good!

void $ rdKafkaAssign kptr pls'

mbq <- getRdMsgQueue $ getKafkaConf k
case mbq of
Expand All @@ -80,10 +89,10 @@ setRebalanceCallback f k e pls = do
forM_ ps (\tp -> redirectPartitionQueue (getKafka k) (tpTopicName tp) (tpPartition tp) mq)
void $ rdKafkaResumePartitions kptr pls

f k (RebalanceAssign assignment)
void $ f k (RebalanceAssign assigned)

KafkaResponseError RdKafkaRespErrRevokePartitions -> do
f k (RebalanceBeforeRevoke assignment)
void $ f k (RebalanceBeforeRevoke ps)
void $ newForeignPtr_ nullPtr >>= rdKafkaAssign kptr
f k (RebalanceRevoke assignment)
void $ f k (RebalanceRevoke ps)
x -> error $ "Rebalance: UNKNOWN response: " <> show x
10 changes: 5 additions & 5 deletions src/Kafka/Consumer/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,20 @@ newtype ConsumerGroupId = ConsumerGroupId
newtype Offset = Offset { unOffset :: Int64 } deriving (Show, Eq, Ord, Read, Generic)

-- | Where to reset the offset when there is no initial offset in Kafka
--
--
-- See <https://kafka.apache.org/documentation/#auto.offset.reset Kafka documentation on offset reset>
data OffsetReset = Earliest | Latest deriving (Show, Eq, Generic)

-- | A set of events which happen during the rebalancing process
data RebalanceEvent =
-- | Happens before Kafka Client confirms new assignment
RebalanceBeforeAssign [(TopicName, PartitionId)]
RebalanceBeforeAssign [TopicPartition]
-- | Happens after the new assignment is confirmed
| RebalanceAssign [(TopicName, PartitionId)]
| RebalanceAssign [TopicPartition]
-- | Happens before Kafka Client confirms partitions rejection
| RebalanceBeforeRevoke [(TopicName, PartitionId)]
| RebalanceBeforeRevoke [TopicPartition]
-- | Happens after the rejection is confirmed
| RebalanceRevoke [(TopicName, PartitionId)]
| RebalanceRevoke [TopicPartition]
deriving (Eq, Show, Generic)

-- | The partition offset
Expand Down