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

Added callbacks to purge cache #15092

Conversation

manojnaidu619
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

When a new article was created, then the existing articles cache was not getting flushed. Because of which, newly created article was not showing up in the response until that stale cache was purged by some other action.

Proposed Solution

Added different callbacks which performs under various stages of article lifecycle to purge cache data and maintain consistency.

  • after_create - it invokes :purge_all and clears existing cache collection. So that, next request would hit server and stores fresh copy of data as cache.
  • after_save - It invokes :purge and purges only itself from cache collection. (like, when an article gets updated)
  • after_destroy - It invokes both :purge and :purge_all clearing record and also the collection holding that record.

Article creation, updation and destruction happens at different parts of application. In order to stay consistent, attached these purge actions to activerecord callbacks.

Related Tickets & Documents

This PR is related to issue #6417

QA Instructions, Screenshots, Recordings

NOTE - It is not possible to test the functionality in local environment. We need to deploy to live server to verify the changes.

Steps to validate:

  • After an article is created/updated/destroyed, next request involving article(s) retrieval will have x-cache: MISS in its response headers.
    • This is because, the stale cache was purged and response was fetched from server.
  • Next consecutive requests will now have x-cache: HIT as it is served from cache.
  • Not all endpoints will show this behaviour. Those endpoints which set surrogate_key inside response headers will respond to cache miss/hit. Rest all endpoints related to articles will always contain x-cache: MISS.
  • Endpoints which set surrogate_key in response headers are...
    • https://dev.to/api/articles
    • https://dev.to/api/articles/:id
    • https://dev.to/api/articles/:slug

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs or
    Storybook (for Crayons components)
  • This PR changes the Forem platform and our documentation needs to be
    updated. I have filled out the
    Changes Requested
    issue template so Community Success can help update the Admin Docs
    appropriately.
  • I've updated the README or added inline documentation
  • I've added an entry to
    CHANGELOG.md
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not: please
    replace this line with details on why this change doesn't need to be
    shared

Are there any post deployment tasks we need to perform?

Yup, validating this fix. Making sure data is purged after creation/updation/destruction of an article by monitoring x-cache key of the request(s) made to those endpoints which set and deliver surrogate_key inside response headers.

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 16, 2021
@github-actions
Copy link
Contributor

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

@citizen428
Copy link
Contributor

Requested a review from @benhalpern and @atsmith813 as they are generally most familiar with all things caching.

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

The absence of purge_all is on purpose. We just don't want to purge all articles when we save one... We have other specific logic here.

I won't entirely close this because maybe there is a conversation to be had about cache logic — there are definitely improvements to be made in a fairly convoluted system, but I don't think this is what we want.

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Nov 1, 2021
@manojnaidu619
Copy link
Contributor Author

@benhalpern there is definitely a scope for discussion on this ticket. But, I cannot able to continue on it at this moment.
It was an amazing learning through out 🙂

@msarit
Copy link
Contributor

msarit commented Nov 22, 2021

@manojnaidu619 @benhalpern @atsmith813 hello folks! 👋🏾
Are y'all still having the caching discussion on this PR?

@citizen428
Copy link
Contributor

Since the original author stated they will no longer be able to work on this so I'll close this PR now and we can have the caching discussion elsewhere if needed.

@citizen428 citizen428 closed this Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants