Skip to content

Fix event recorder (GSI-691)#97

Merged
TheByronHimes merged 1 commit into
mainfrom
feature/patch_kafka_reset_GSI-691
Mar 24, 2024
Merged

Fix event recorder (GSI-691)#97
TheByronHimes merged 1 commit into
mainfrom
feature/patch_kafka_reset_GSI-691

Conversation

@TheByronHimes

Copy link
Copy Markdown
Member

Fixes an problem that can arise when using the EventRecorder test utility for kafka in combination with the clear_topics reset function, which resulted in a infinitely running consumer. The sequence that caused the problem was roughly as follows:

  1. An EventRecorder instance is used to consume an event in testing. This establishes the consumer and its offsets.
  2. Another N events are published, increasing the log end offset. These are not consumed by the event recorder.
  3. clear_topics() is called, deleting the records but leaving the offsets.
  4. The event recorder is used again to consume < N events. When the block of code inside the event recorder context manager is finished, the event recorder will evaluate offsets again. It will take the initial offsets (established in step 1) and subtract that from the current topic-end offsets (inflated in step 2). Even though <N events were published inside the block, the event recorder will try to consume at least N events. That means it waits forever because N events never arrive, only <N.

Two fixes are available, and I went with the simpler one for now, which is to make the consumer seek to the end of topics upon entering the context. That way, only the events published inside the context are considered, which is the expected behavior.
The other fix would be to use an additional kafka CLI tools command inside the clear_topics function to directly reset offsets, but I'm not sure about all the ramifications of that right now.

@TheByronHimes
TheByronHimes requested review from Cito and mephenor and removed request for mephenor March 21, 2024 13:26

@Cito Cito left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot for looking into this.

The fix looks reasonable to me and I confirmed it solves the issue we have in the ARS tests.

I only was a bit confused about the description of the PR, since the problem in the ARS tests does not seem to have anything to do with clear_topics() - as far as I see it is not called directly or indirectly anywhere in these tests.

Also, I would have thought that the parameter auto_offset_reset="latest" would make the seek_to_end() unnecessary. But it seems this parameter is only used in case of an error, not for normal operation. PyKafka has another parameter reset_offset_on_start which might be useful here, but aiokafka doesn't have it.

You can also consider upgrading aiokafka to 0.10.0 while you're at it. We will need this eventually anyway to support Python 3.12 in hexkit.

@TheByronHimes
TheByronHimes merged commit e1255a1 into main Mar 24, 2024
@TheByronHimes
TheByronHimes deleted the feature/patch_kafka_reset_GSI-691 branch March 24, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants