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

Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder #6779

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

leizor
Copy link
Contributor

@leizor leizor commented Nov 29, 2023

What this PR does

The use of BucketScanBlocksFinder was supposed to be removed in #6673 but its counterpart BucketIndexBlocksFinder was accidentally removed instead.

This PR also removes references to the old bucket scan option from the docs.

This reverses that.

Which issue(s) this PR fixes or relates to

#6670

Checklist

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

@leizor leizor requested a review from a team as a code owner November 29, 2023 16:41
@leizor leizor marked this pull request as draft November 29, 2023 16:57
@leizor
Copy link
Contributor Author

leizor commented Nov 29, 2023

This is less straightforward than I expected; marking as a draft for now while I figure out the failing integration tests.

@leizor leizor force-pushed the leizor/fix-bucket-scan-index branch from 3c37d40 to c68fb19 Compare November 29, 2023 20:30
@leizor leizor marked this pull request as ready for review November 29, 2023 21:24
@leizor leizor requested a review from a team as a code owner November 29, 2023 21:24
Copy link
Contributor

@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.

That's great! Screenshot 2023-11-30 at 16 05 51

But I think we can do better. Can you also remove the alert and its runbook

{
// Alert if the querier is not successfully scanning the bucket.
alert: $.alertName('QuerierHasNotScanTheBucket'),
'for': '5m',
expr: |||
(time() - cortex_querier_blocks_last_successful_scan_timestamp_seconds > 60 * 30)
and
cortex_querier_blocks_last_successful_scan_timestamp_seconds > 0
|||,
labels: {
severity: 'critical',
},
annotations: {
message: '%(product)s Querier %(alert_instance_variable)s in %(alert_aggregation_variables)s has not successfully scanned the bucket since {{ $value | humanizeDuration }}.' % $._config,
},
},

After doing that you should probably run make build-mixin build-helm-tests and commit the diff

The use of BucketScanBlocksFinder was supposed to be removed in
#6673 but its counterpart
BucketIndexBlocksFinder was accidentally removed instead.

This reverses that.
@leizor leizor force-pushed the leizor/fix-bucket-scan-index branch from 93b0d6f to 91c93ef Compare November 30, 2023 19:04
@leizor
Copy link
Contributor Author

leizor commented Nov 30, 2023

@dimitarvdimitrov Good call, done!

Copy link
Contributor

@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 so much for doing this

@leizor
Copy link
Contributor Author

leizor commented Nov 30, 2023

Thanks for the reviews!

@leizor leizor merged commit ff8a70a into main Nov 30, 2023
30 checks passed
@leizor leizor deleted the leizor/fix-bucket-scan-index branch November 30, 2023 22:44
grafanabot pushed a commit that referenced this pull request Dec 1, 2023
* Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder

The use of BucketScanBlocksFinder was supposed to be removed in
#6673 but its counterpart
BucketIndexBlocksFinder was accidentally removed instead.

This reverses that.

* Remove BlocksFinderBucketScan

* Update docs wrt. bucket scanning option

* Remove alert and runbook for bucket scanning

(cherry picked from commit ff8a70a)
@pracucci pracucci mentioned this pull request Dec 4, 2023
3 tasks
@grafanabot
Copy link
Contributor

The backport to r267 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-6779-to-r267 origin/r267
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x ff8a70aee6d187113d0f2b18b75144a4bee5091d
# Push it to GitHub
git push --set-upstream origin backport-6779-to-r267
git switch main
# Remove the local backport branch
git branch -D backport-6779-to-r267

Then, create a pull request where the base branch is r267 and the compare/head branch is backport-6779-to-r267.

@pr00se
Copy link
Contributor

pr00se commented Dec 6, 2023

This didn't need to be backported to r267 (the changelog entry was added in #6808)

56quarters added a commit that referenced this pull request Apr 25, 2024
This compactor was added in #6779 but isn't actually needed since we're
testing monolithic mode which already includes the compactor component.

Coincidentally, this also seems to fix #7972

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request Apr 25, 2024
This compactor was added in #6779 but isn't actually needed since we're
testing monolithic mode which already includes the compactor component.

Coincidentally, this also seems to fix #7972

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants