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

Reset frameBytesLeft when cutting a frame in streamChunkedReadResponses #4423

Merged
merged 4 commits into from
Mar 9, 2023

Conversation

leizor
Copy link
Contributor

@leizor leizor commented Mar 8, 2023

What this PR does

This PR adds a reset to frameBytesLeft when streaming chunked read responses. Before this change, streamChunkedReadResponses would start writing one chunk per frame after the initial frame.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Justin Lei <justin.lei@grafana.com>
@leizor leizor force-pushed the leizor/remote-read-frame-size-reset branch from 0915c96 to 9a854b5 Compare March 8, 2023 17:28
@leizor leizor marked this pull request as ready for review March 8, 2023 17:51
@leizor leizor requested a review from a team as a code owner March 8, 2023 17:51
Copy link
Contributor

@treid314 treid314 left a comment

Choose a reason for hiding this comment

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

Good fix! Logically this makes sense, just some style comments.

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@

### Grafana Mimir

* [BUGFIX] Querier: Streaming remote read was returning 1 chunk per frame after the first frame of a series. #4423
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be sorted with the other [BUGFIX] tags at the bottom of the changelog file. Can you clarify the comment a bit, something like "Streaming read will return multiple chunks per frame after the first frame."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oop, didn't notice they were sorted--fixed!

pkg/querier/remote_read_test.go Outdated Show resolved Hide resolved
pkg/querier/remote_read.go Outdated Show resolved Hide resolved
leizor and others added 3 commits March 8, 2023 14:40
Signed-off-by: Justin Lei <justin.lei@grafana.com>
Co-authored-by: Tyler Reid <tyler.reid@grafana.com>
Co-authored-by: Tyler Reid <tyler.reid@grafana.com>
@leizor leizor force-pushed the leizor/remote-read-frame-size-reset branch from 347e10d to a1247a8 Compare March 8, 2023 22:40
Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM

@krajorama krajorama merged commit 74c6d77 into main Mar 9, 2023
@krajorama krajorama deleted the leizor/remote-read-frame-size-reset branch March 9, 2023 08:19
osg-grafana pushed a commit that referenced this pull request Mar 9, 2023
…es (#4423)

* Reset frameBytesLeft when cutting a frame in streamChunkedReadResponses
* Update CHANGELOG.md
* Rename frameBytesLeft -> frameBytesRemaining
* Update pkg/querier/remote_read_test.go

Signed-off-by: Justin Lei <justin.lei@grafana.com>
Co-authored-by: Tyler Reid <tyler.reid@grafana.com>
pstibrany added a commit that referenced this pull request Mar 9, 2023
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
pstibrany added a commit that referenced this pull request Mar 9, 2023
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
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.

3 participants