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

Avoid making delete requests when deletes are disabled for a user #6583

Conversation

MasslessParticle
Copy link
Contributor

@MasslessParticle MasslessParticle commented Jul 5, 2022

Originally, when deletes were disabled, the querier didn't make a request at all. This change emulates that and only makes delete requests for tenants who have deletes enabled.

Copy link
Contributor

@DylanGuedes DylanGuedes left a comment

Choose a reason for hiding this comment

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

lgtm, just a nit

@grafanabot
Copy link
Collaborator

./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.4%

@cstyan
Copy link
Contributor

cstyan commented Jul 5, 2022

Is the enable/disable of the deletes for a tenant runtime reloadable? if not it might make more sense to have a noopDeleteClient?

@MasslessParticle
Copy link
Contributor Author

It's just part of overrides so I think it's runtime reloadable.

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, with a nit

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

@MasslessParticle MasslessParticle merged commit a92e048 into grafana:main Jul 6, 2022
grafanabot pushed a commit that referenced this pull request Jul 6, 2022
@MasslessParticle MasslessParticle added the backport release-2.6.x Tag a PR with this label to create a PR which cherry pics it into the release-2.6.x branch label Jul 14, 2022
@grafanabot
Copy link
Collaborator

The backport to release-2.6.x 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-6583-to-release-2.6.x origin/release-2.6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x a92e048ec31148fa709e85dfd700f4cf5f58b8b0
# Push it to GitHub
git push --set-upstream origin backport-6583-to-release-2.6.x
git switch main
# Remove the local backport branch
git branch -D backport-6583-to-release-2.6.x

Then, create a pull request where the base branch is release-2.6.x and the compare/head branch is backport-6583-to-release-2.6.x.

MasslessParticle added a commit that referenced this pull request Jul 14, 2022
@MasslessParticle MasslessParticle added backport release-2.6.x Tag a PR with this label to create a PR which cherry pics it into the release-2.6.x branch and removed backport release-2.6.x Tag a PR with this label to create a PR which cherry pics it into the release-2.6.x branch labels Jul 14, 2022
@grafanabot
Copy link
Collaborator

The backport to release-2.6.x 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-6583-to-release-2.6.x origin/release-2.6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x a92e048ec31148fa709e85dfd700f4cf5f58b8b0
# Push it to GitHub
git push --set-upstream origin backport-6583-to-release-2.6.x
git switch main
# Remove the local backport branch
git branch -D backport-6583-to-release-2.6.x

Then, create a pull request where the base branch is release-2.6.x and the compare/head branch is backport-6583-to-release-2.6.x.

vlad-diachenko pushed a commit that referenced this pull request Jul 15, 2022
func (c *perTenantDeleteRequestsClient) GetAllDeleteRequestsForUser(ctx context.Context, userID string) ([]DeleteRequest, error) {
allLimits := c.limits.AllByUserID()
userLimits, ok := allLimits[userID]
if ok && userLimits.CompactorDeletionEnabled || c.limits.DefaultLimits().CompactorDeletionEnabled {
Copy link
Contributor

@vlad-diachenko vlad-diachenko Jul 15, 2022

Choose a reason for hiding this comment

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

@MasslessParticle it looks like if we have CompactorDeletionEnabled=true in defaultLimits then even if we disable deletion for this particular tenant, it will be ignored and deletion will be processed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.6.x Tag a PR with this label to create a PR which cherry pics it into the release-2.6.x branch size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants