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

Remove mmap-based index header reader #4280

Merged
merged 10 commits into from
Feb 24, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Feb 23, 2023

What this PR does

This PR removes the mmap-based index header reader from the store gateway, and makes the mmap-less index header reader the default.

It also removes the fileutil package, which contained only mmap-related functionality that is no longer used anywhere.

Only thing I was unsure about here was whether or not we want to remove the -blocks-storage.bucket-store.index-header.stream-reader-enabled flag immediately, or keep it (but ignore it) until the major release after the next major release.

Which issue(s) this PR fixes or relates to

#3465

Checklist

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

@charleskorn charleskorn marked this pull request as ready for review February 23, 2023 05:02
@charleskorn charleskorn requested review from a team as code owners February 23, 2023 05:02
This functionality is now exercised by the other integration tests
given the mmap-less index-header reader is now the default.
Copy link
Collaborator

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

LGTM (modulo few comments)! Also, before merging, can you also remove the flag reference from docs/sources/mimir/operators-guide/configure/about-versioning.md?

Only thing I was unsure about here was whether or not we want to remove the -blocks-storage.bucket-store.index-header.stream-reader-enabled flag immediately, or keep it (but ignore it) until the major release after the next major release.

It was experimental, so it's OK to remove it without deprecating it first.

pkg/storegateway/indexheader/header.go Outdated Show resolved Hide resolved
pkg/storegateway/indexheader/header.go Outdated Show resolved Hide resolved
@charleskorn
Copy link
Contributor Author

Also, before merging, can you also remove the flag reference from docs/sources/mimir/operators-guide/configure/about-versioning.md?

Done.

I've also renamed the stream-reader-max-idle-file-handles flag to max-idle-file-handles given there's now no other kind of reader.

@charleskorn charleskorn force-pushed the charleskorn/remove-mmap-index-header-reader branch from aca6f60 to 50c1909 Compare February 24, 2023 00:23
Copy link
Collaborator

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

Thanks!

@pracucci pracucci merged commit 9e17e00 into main Feb 24, 2023
@pracucci pracucci deleted the charleskorn/remove-mmap-index-header-reader branch February 24, 2023 07:56
56quarters added a commit that referenced this pull request May 16, 2024
Per #4280, we removed used of mmap. This change removes references to it
in architecture documentation. It also removes references to features that
have been removed such as not using the bucket index.

Fixes #8145

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request May 16, 2024
Per #4280, we removed use of mmap. This change removes references to it
in architecture documentation. It also removes references to features that
have been removed such as not using the bucket index.

Fixes #8145

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request May 20, 2024
* docs: Remove references to mmap in store-gateway

Per #4280, we removed use of mmap. This change removes references to it
in architecture documentation. It also removes references to features that
have been removed such as not using the bucket index.

Fixes #8145

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Code review

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

---------

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
francoposa pushed a commit that referenced this pull request May 27, 2024
* docs: Remove references to mmap in store-gateway

Per #4280, we removed use of mmap. This change removes references to it
in architecture documentation. It also removes references to features that
have been removed such as not using the bucket index.

Fixes #8145

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Code review

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

---------

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants