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

Fix some cases where flush reason is recorded as 'Immediate' #2022

Merged
merged 2 commits into from
Jan 23, 2020

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Jan 22, 2020

Pass the reason from shouldFlushChunk() to closeHead(); sending reasonImmediate was a bug introduced in #1978.

We don't need to check len(chunks) is non-zero, since that would return noFlush from shouldFlushSeries().

Also close the gap where reasonImmediate could be returned on an empty series, which would then have crashed. Don't know if it's possible in practice to get that state.

Signed-off-by: Bryan Boreham <bryan@weave.works>
Pass the reason from `shouldFlushChunk()` to `closeHead()`;
sending `reasonImmediate` was a bug.

We don't need to check `len(chunks)` is non-zero, since that would
return `noFlush` from `shouldFlushSeries()`.

Signed-off-by: Bryan Boreham <bryan@weave.works>
@bboreham bboreham changed the title Fix flush immediate Fix some cases where flush reason is recorded as 'Immediate' Jan 22, 2020
@bboreham bboreham mentioned this pull request Jan 22, 2020
3 tasks
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with the ingester flush logic, so I took a look at the internals. To my understanding, this PR LGTM.

codesome added a commit to grafana/cortex that referenced this pull request Jan 23, 2020
commit df8b95f
Author: Bryan Boreham <bryan@weave.works>
Date:   Wed Jan 22 13:52:44 2020 +0000

    Pass the correct flush reason to closeHead()

    Pass the reason from `shouldFlushChunk()` to `closeHead()`;
    sending `reasonImmediate` was a bug.

    We don't need to check `len(chunks)` is non-zero, since that would
    return `noFlush` from `shouldFlushSeries()`.

    Signed-off-by: Bryan Boreham <bryan@weave.works>

commit 17c0c21
Author: Bryan Boreham <bryan@weave.works>
Date:   Wed Jan 22 13:45:22 2020 +0000

    Make sure we return noFlush on an empty series

    Signed-off-by: Bryan Boreham <bryan@weave.works>

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM

@bboreham bboreham merged commit 62efd50 into master Jan 23, 2020
@bboreham bboreham deleted the fix-flush-immediate branch January 23, 2020 08:53
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