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

backporting fork changes to v1.17.1 #5

Merged
merged 7 commits into from
Oct 3, 2024

Conversation

ortuman
Copy link
Collaborator

@ortuman ortuman commented Oct 3, 2024

brings the following commits to v1.17.1 (twmb@8b955b4)

dimitarvdimitrov and others added 6 commits October 3, 2024 10:41
…tch responses

### Background

In grafana/mimir we are working towards making fetch requests ourselves. The primary reason behind that is that individual requests to the kafka backend are slow, so doing them sequentially per partition becomes the bottleneck in our application. So we want to fetch records in parallel to speed up the consumption.

One difficulty I met when issuing `FetchRequest`s ourselves is that parsing the response is non-trivial. That's why I'm proposing to export these functions for downstream projects to use.

Alternatively, I can also try contributing the concurrent fetching logic. But I believe that is much more nuanced and with more tradeoffs around fetched bytes and latency. So I wasn't sure whether it's a good fit for a general purpose library. I'm open to discuss this further.

### What this PR does

Moves `(*kgo.cursorOffsetNext).processRespPartition` from being a method to being a standalone function - `kgo.processRespPartition`. There were also little changes necessary to make the interface suitable for public use (like removing the `*broker` parameter).

### Side effects

To minimize the necessary changes and the API surface of the package I opted to use a single global decompressor for all messages. Previously, there would be one decompressor per client and that decompressor would be passed down to `(*cursorOffsetNext).processRespPartition`. My understanding is that using different pooled readers (lz4, zst, gzip) shouldn't have a negative impact on performance because usage patterns do not affect the behaviour of the reader (for example, a consistent size of decompressed data doesn't make the reader more or less efficient). I have not thoroughly verified or tested this - Let me know if you think that's important.

An alternative to this is to also export the `decompressor` along with `newDecompressor()` and the auxiliary types for decompression.
Signed-off-by: Miguel Ángel Ortuño <ortuman@gmail.com>
…th-export-partition-parsing-utils

fetching: export utilities for decompressing and parsing partition fetch responses
@ortuman ortuman changed the title backporting fork changes to v1.17.1 backporting fork changes to v1.17.1 Oct 3, 2024
@ortuman ortuman marked this pull request as ready for review October 3, 2024 09:04
…tions

Signed-off-by: Miguel Ángel Ortuño <ortuman@gmail.com>
@ortuman ortuman force-pushed the ortuman/backport-fork-changes-to-v1.17.1 branch from d5fa5fe to 6010ccd Compare October 3, 2024 10:18
Copy link

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. I cherry-picked the 3 merge commits onto 1.17.1 and there weren't any diffs between the two versions

@ortuman ortuman merged commit f8f76a4 into v1.17.1 Oct 3, 2024
1 check passed
@ortuman ortuman deleted the ortuman/backport-fork-changes-to-v1.17.1 branch October 17, 2024 08:32
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