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(blooms): dont break iterator conventions #12808

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Apr 26, 2024

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

@owen-d owen-d requested a review from a team as a code owner April 26, 2024 19:59
refactors skip logic for bloom pages that are too large

s/Seek/LoadOffset/ for LazyBloomIter

removes unused code
Signed-off-by: Owen Diehl <[email protected]>
@owen-d owen-d force-pushed the blooms/better-skipping-large-pages branch from adbbaf0 to 3ca36fa Compare April 26, 2024 20:03
@@ -144,14 +144,12 @@ func (bq *BlockQuerier) Seek(fp model.Fingerprint) error {
func (bq *BlockQuerier) Next() bool {
for bq.series.Next() {
series := bq.series.At()
bq.blooms.Seek(series.Offset)
if ok := bq.blooms.LoadOffset(series.Offset); !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think LoadOffset should return an error. Then we should check here if the error is due to the page being too big in which case we can safely continue. Otherwise, we wont be able to error when there is. for example, an IO error.

fq.bq.blooms.Seek(series.Offset)
ok := fq.bq.blooms.LoadOffset(series.Offset)
if !ok {
// could not seek to the desired bloom,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment

@owen-d
Copy link
Member Author

owen-d commented Apr 29, 2024

Good points @salvacorts -- I inverted the logic and now LoadOffset returns a skip:

// LoadOffset returns whether the bloom page at the given offset should
// be skipped (due to being too large) _and_ there's no error
func (it *LazyBloomIter) LoadOffset(offset BloomOffset) (skip bool)

It'll only return true when it's safe to skip so other times the error will surface immediately after.

@owen-d owen-d force-pushed the blooms/better-skipping-large-pages branch from 638b3e5 to 5575682 Compare April 29, 2024 16:49
@owen-d owen-d force-pushed the blooms/better-skipping-large-pages branch from 5575682 to fc6554b Compare April 29, 2024 17:01
@chaudum chaudum added type/bug Somehing is not working as expected backport k200 labels Apr 30, 2024
@grafanabot
Copy link
Collaborator

This PR must be merged before a backport PR will be created.

1 similar comment
@grafanabot
Copy link
Collaborator

This PR must be merged before a backport PR will be created.

@chaudum chaudum merged commit 1665e85 into grafana:main Apr 30, 2024
61 checks passed
grafanabot pushed a commit that referenced this pull request Apr 30, 2024
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)
shantanualsi pushed a commit that referenced this pull request May 6, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k200 size/L type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants