-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Compactor: add per tenant compaction delete enabled flag #6410
Compactor: add per tenant compaction delete enabled flag #6410
Conversation
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0.3%
+ querier 0%
- querier/queryrange -0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
Would it be cleaner to just make a middleware to wrap our handlers? If we took that approach, we wouldn't have to implement this same check in every handler. |
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Yes, it's been added. It even has unit tests :) |
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0.3%
+ querier 0%
+ querier/queryrange 0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
This looks good! Were we going remove |
Due to the size of this one I was going to do it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two usability nits, otherwise looks good
pkg/storage/stores/shipper/compactor/deletion/request_handler.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @MichelHollands 👍
The docs changes here are fairly minor @KMiller-Grafana so I'm going to merge, I hope you don't mind?
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
- distributor -0.3%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
- querier/queryrange -0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-6410-to-k102 origin/k102
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x b4e6c599368278b8da20852a9acc4722f70af603
# Push it to GitHub
git push --set-upstream origin backport-6410-to-k102
git switch main
# Remove the local backport branch
git branch -D backport-6410-to-k102 Then, create a pull request where the |
* Add per tenant compaction delete enabled flag Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Remove changes in wrong place Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add compactor deletion enabled field Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use limit in compactor Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use http middleware and add test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Fix lint issue Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Revert to default setting if no override Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add default value command line option Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update the docs Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Enable access to deletion API for integration test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Rename flag to allow_deletes Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update per review comments Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit b4e6c59)
* Compactor: add per tenant compaction delete enabled flag (#6410) * Add per tenant compaction delete enabled flag Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Remove changes in wrong place Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add compactor deletion enabled field Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use limit in compactor Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use http middleware and add test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Fix lint issue Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Revert to default setting if no override Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add default value command line option Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update the docs Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Enable access to deletion API for integration test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Rename flag to allow_deletes Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update per review comments Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit b4e6c59) * Fix changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
* Compactor: add per tenant compaction delete enabled flag (#6410) * Add per tenant compaction delete enabled flag Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Remove changes in wrong place Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add compactor deletion enabled field Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use limit in compactor Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use http middleware and add test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Fix lint issue Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Revert to default setting if no override Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add default value command line option Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update the docs Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Enable access to deletion API for integration test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Rename flag to allow_deletes Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update per review comments Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit b4e6c59) * Fix changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit df48f93)
…) (#6496) * Compactor: add per tenant compaction delete enabled flag (#6410) * Add per tenant compaction delete enabled flag Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Remove changes in wrong place Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add compactor deletion enabled field Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use limit in compactor Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Use http middleware and add test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Fix lint issue Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Revert to default setting if no override Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Add default value command line option Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update the docs Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Enable access to deletion API for integration test Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Rename flag to allow_deletes Signed-off-by: Michel Hollands <michel.hollands@grafana.com> * Update per review comments Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit b4e6c59) * Fix changelog Signed-off-by: Michel Hollands <michel.hollands@grafana.com> (cherry picked from commit df48f93) Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
Signed-off-by: Michel Hollands michel.hollands@grafana.com
What this PR does / why we need it:
This PR adds a per tenant compaction delete enabled flag called
compactor_deletion_enabled
. A unit test was added for the http middleware that checks access.This was also manually tested. The following snippet was added in the Loki config file:
The runtime config contained the following:
Running curl commands against the API gives the expected results:
Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md