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

Bump frontend results cache version #1631

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Conversation

colega
Copy link
Contributor

@colega colega commented Apr 5, 2022

What this PR does

Added a cache.Versioned wrapper and used it to bump the frontend's result cache version, which is needed to invalidate results that might have been cached by a previous version of the code.

Which issue(s) this PR fixes or relates to

Fixes #1625

Checklist

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

@colega colega marked this pull request as ready for review April 5, 2022 11:56
@colega colega requested a review from a team April 6, 2022 07:22
Copy link
Member

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

@colega colega force-pushed the bump-frontend-results-cache-version branch from 02935cd to 3789db9 Compare April 6, 2022 07:48
@colega colega enabled auto-merge (squash) April 6, 2022 07:48
Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

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

Neat approach, I like it.

What the thought process for downgrading? Or more commonly, rolling back in a panic scenario. You could argue that there is no need to invalidate when going back a version, but I don't think it is worth spending time on.

[nit] Would it be worth somehow better highlighting cache invalidation in the CHANGELOG? Do we have a convention for putting important notices at the top of the file? It is definitely something an operator will want to be aware of when rolling out the release. I'm imagining the scenario: "queries got slower after I rolled out xyz.."

@colega
Copy link
Contributor Author

colega commented Apr 6, 2022

What the thought process for downgrading? Or more commonly, rolling back in a panic scenario. You could argue that there is no need to invalidate when going back a version, but I don't think it is worth spending time on.

Downgrading version doesn't require any extra thoughts, the previous keys might still be there (or maybe not, if they were evicted by memcached's decision) but key space is disjoint so nothing special is required.

Would it be worth somehow better highlighting cache invalidation in the CHANGELOG? Do we have a convention for putting important notices at the top of the file? It is definitely something an operator will want to be aware of when rolling out the release. I'm imagining the scenario: "queries got slower after I rolled out xyz.."

I've added it at [CHANGE] section, I'd expect the operators to check at least that section.

Added a cache.Versioned wrapper and used it to bump the frontend's
result cache version, which is needed to invalidate results that might
have been cached by a previous version of the code.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega force-pushed the bump-frontend-results-cache-version branch from 7a31120 to cee995f Compare April 6, 2022 09:30
@colega colega merged commit 91b3c98 into main Apr 6, 2022
@colega colega deleted the bump-frontend-results-cache-version branch April 6, 2022 09:38
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.

Version the caches and increase version when bugs are fixed
3 participants