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.

Sign up for GitHub

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

Jump to bottom

fix(blooms): Reset error on LazyBloomIter.Seek #12806

Merged
merged 2 commits into from
Apr 26, 2024
Merged

fix(blooms): Reset error on LazyBloomIter.Seek #12806

merged 2 commits into from
Apr 26, 2024

Conversation

salvacorts
Copy link
Contributor

What this PR does / why we need it:

We are skipping all pages after any of the pages on a given block have failed due to being too big:

  1. Seek a page that is too big. it.err is set here

    loki/pkg/storage/bloom/v1/bloom_querier.go

    Lines 66 to 68 in feb210b

    decoder, err := it.b.blooms.BloomPageDecoder(r, offset.Page, it.m, it.b.metrics, series...)
    if err != nil {
    it.err = errors.Wrap(err, "loading bloom page")

  2. then it.Next return false in this check

    loki/pkg/storage/bloom/v1/bloom_querier.go

    Lines 87 to 89 in feb210b

    func (it *LazyBloomIter) next(series ...model.Fingerprint) bool {
    if it.err != nil {
    return false
    .

  3. Seek another page that should be valid. Decoding of the new page succeeds but Seek won’t reset it.err. Therefore, the call to it.Next return false again.

This PR fixes this by resetting it.err on every Seek.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
chaudum
chaudum reviewed Apr 26, 2024
View reviewed changes
pkg/storage/bloom/v1/bloom_querier.go Outdated
@@ -45,6 +45,9 @@ func (it *LazyBloomIter) ensureInit() {
func (it *LazyBloomIter) Seek(offset BloomOffset) {
it.ensureInit()

// reset error from any previous seek/next
it.err = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should reset the error unconditionally, e.g we should not continue seeking after IO error.
Instead, we probably want to check the error type, similar to what the do here https://github.com/grafana/loki/pull/12297/files#diff-ad299f9ea738f6f6d8e1a3b9fcc651506b3a241dbfbd95a60ab9d96b5b8b4d4aR156
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Thank you!
@salvacorts salvacorts marked this pull request as ready for review April 26, 2024 15:24
@salvacorts salvacorts requested a review from a team as a code owner April 26, 2024 15:24
@salvacorts salvacorts enabled auto-merge (squash) April 26, 2024 15:33
chaudum
chaudum approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM
pkg/storage/bloom/v1/fuse_test.go

if fp == 0 || fp == 2 {
require.False(t, querier.blooms.Next())
require.Error(t, querier.blooms.Err())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also check the error type here?
@salvacorts salvacorts merged commit 76ba24e into main Apr 26, 2024
58 checks passed
@salvacorts salvacorts deleted the salvacorts/fix-resetting-error-LazyBloomIter branch April 26, 2024 17:11
@owen-d owen-d mentioned this pull request Apr 26, 2024
Merged
@loki-gh-app loki-gh-app bot mentioned this pull request Apr 29, 2024
Open
chaudum pushed a commit that referenced this pull request Apr 30, 2024
@owen-d
fix(blooms): dont break iterator conventions (#12808)
1665e85 
Followup to #12806 which exposes skipped pages more explicitly than as an error.

* refactors skip logic for bloom pages that are too large
* s/Seek/LoadOffset/ for LazyBloomIter
* removes unused code
grafanabot pushed a commit that referenced this pull request Apr 30, 2024
@owen-d @grafana-delivery-bot
fix(blooms): dont break iterator conventions (#12808)
957d6a4 
Followup to #12806 which exposes skipped pages more explicitly than as an error.

* refactors skip logic for bloom pages that are too large
* s/Seek/LoadOffset/ for LazyBloomIter
* removes unused code

(cherry picked from commit 1665e85)
@grafanabot grafanabot mentioned this pull request Apr 30, 2024
Merged
shantanualsi pushed a commit that referenced this pull request May 6, 2024
@owen-d @shantanualsi
fix(blooms): dont break iterator conventions (#12808)
22a904c 
Followup to #12806 which exposes skipped pages more explicitly than as an error.

* refactors skip logic for bloom pages that are too large
* s/Seek/LoadOffset/ for LazyBloomIter
* removes unused code
This was referenced May 6, 2024
Open
Open
@loki-gh-app loki-gh-app bot mentioned this pull request May 20, 2024
Open
This was referenced May 27, 2024
Open
Open
This was referenced Jun 10, 2024
Open
Merged
@loki-gh-app loki-gh-app bot mentioned this pull request Jun 24, 2024
Open
@loki-gh-app loki-gh-app bot mentioned this pull request Jul 1, 2024
Open
@grafanabot grafanabot mentioned this pull request Jul 2, 2024
Open
This was referenced Jul 3, 2024
Open
Closed
Open
@RodrigoCMoraes RodrigoCMoraes mentioned this pull request Jul 12, 2024
Closed
@loki-gh-app loki-gh-app bot mentioned this pull request Jul 15, 2024
Open
@loki-gh-app loki-gh-app bot mentioned this pull request Jul 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L
2 participants
@salvacorts @chaudum