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

Revert "Introduces cache to TSDB postings (#9621)" #11157

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Nov 7, 2023

This reverts commit 1221658.

What this PR does / why we need it:
We decided to discontinue the postings cache a few releases ago for not finding any evidences that postings was the culprit for the bad igw performance. Plus, we noticed a major issue with it: it doesn't bubble down shards correctly, so every query end up processing every shard.

My approach here was:

  1. Revert the commit that introduced TSDB postings cache
  2. Compare what changed after reverting with what we had before introducing PR 9621. If everything that changed was something introduced by PR 9621, move forward with reverting the code. Otherwise, evaluate if the change won't introduce undesired behavior
  3. Delete deadcode (ex: postings_cache and codecs files)

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Nov 7, 2023
@DylanGuedes DylanGuedes force-pushed the remove-postingscache-and-fix-sharding-usage branch 5 times, most recently from f386f95 to d70acc1 Compare November 7, 2023 16:07
@DylanGuedes DylanGuedes force-pushed the remove-postingscache-and-fix-sharding-usage branch 11 times, most recently from 2c5ef19 to b111488 Compare November 7, 2023 17:16
@DylanGuedes DylanGuedes force-pushed the remove-postingscache-and-fix-sharding-usage branch from b111488 to abaf558 Compare November 7, 2023 17:31
@DylanGuedes DylanGuedes marked this pull request as ready for review November 7, 2023 17:38
@DylanGuedes DylanGuedes requested a review from a team as a code owner November 7, 2023 17:38
@DylanGuedes DylanGuedes added the type/bug Somehing is not working as expected label Nov 7, 2023
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

LGTM

@DylanGuedes DylanGuedes merged commit 30fbe38 into main Nov 7, 2023
8 checks passed
@DylanGuedes DylanGuedes deleted the remove-postingscache-and-fix-sharding-usage branch November 7, 2023 20:03
@grafanabot
Copy link
Collaborator

The backport to release-2.8.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-11157-to-release-2.8.x origin/release-2.8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 30fbe383411c56e0d7a6be11e2333ab2513a89c0

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-release-2.8.x
# Create the PR body template
PR_BODY=$(gh pr view 11157 --json body --template 'Backport 30fbe383411c56e0d7a6be11e2333ab2513a89c0 from #11157{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[release-2.8.x] Revert \"Introduces cache to TSDB postings (#9621)\"" --body-file - --label "size/XXL" --label "type/docs" --label "type/bug" --label "backport" --base release-2.8.x --milestone release-2.8.x --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-release-2.8.x

# Create a pull request where the `base` branch is `release-2.8.x` and the `compare`/`head` branch is `backport-11157-to-release-2.8.x`.

# Remove the local backport branch
git switch main
git branch -D backport-11157-to-release-2.8.x

@grafanabot
Copy link
Collaborator

The backport to release-2.9.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-11157-to-release-2.9.x origin/release-2.9.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 30fbe383411c56e0d7a6be11e2333ab2513a89c0

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-release-2.9.x
# Create the PR body template
PR_BODY=$(gh pr view 11157 --json body --template 'Backport 30fbe383411c56e0d7a6be11e2333ab2513a89c0 from #11157{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[release-2.9.x] Revert \"Introduces cache to TSDB postings (#9621)\"" --body-file - --label "size/XXL" --label "type/docs" --label "type/bug" --label "backport" --base release-2.9.x --milestone release-2.9.x --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-release-2.9.x

# Create a pull request where the `base` branch is `release-2.9.x` and the `compare`/`head` branch is `backport-11157-to-release-2.9.x`.

# Remove the local backport branch
git switch main
git branch -D backport-11157-to-release-2.9.x

@grafanabot
Copy link
Collaborator

The backport to k174 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-11157-to-k174 origin/k174
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 30fbe383411c56e0d7a6be11e2333ab2513a89c0

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-k174
# Create the PR body template
PR_BODY=$(gh pr view 11157 --json body --template 'Backport 30fbe383411c56e0d7a6be11e2333ab2513a89c0 from #11157{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[k174] Revert \"Introduces cache to TSDB postings (#9621)\"" --body-file - --label "size/XXL" --label "type/docs" --label "type/bug" --label "backport" --base k174 --milestone k174 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-11157-to-k174

# Create a pull request where the `base` branch is `k174` and the `compare`/`head` branch is `backport-11157-to-k174`.

# Remove the local backport branch
git switch main
git branch -D backport-11157-to-k174

DylanGuedes added a commit that referenced this pull request Nov 7, 2023
**What this PR does / why we need it**:
Backport #11157 into k174
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
)

This reverts commit 1221658.

**What this PR does / why we need it**:
We decided to discontinue the postings cache a few releases ago for not
finding any evidences that postings was the culprit for the bad igw
performance. Plus, we noticed a major issue with it: it doesn't bubble
down shards correctly, so every query end up processing every shard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k174 backport release-2.9.x backport-failed size/XXL type/bug Somehing is not working as expected type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants