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

Support deferred expiration of records and attributes for one-to-many associations #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drinkbeer
Copy link
Contributor

Related issues: https://github.com/Shopify/caching-platform/issues/708

  • Deferred the expiration of records and attributes to the end of transaction
  • Used batch operations (delete_multi/write_multi) instead of expire each record in a loop
  • In dev.yml, create the test database identity_cache_test in Mysql, achieved it by adding a Rake task
  • Adding pry as a dependency to the repo

@drinkbeer drinkbeer requested review from kirs, Stivaros and a team October 6, 2024 21:49
@danmayer
Copy link

danmayer commented Oct 7, 2024

nice looking pretty good so far... Why did you need to add exists? that wasn't clear to me on first read

Copy link
Contributor

@Stivaros Stivaros left a comment

Choose a reason for hiding this comment

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

Really excited to see this feature coming along!

@@ -240,6 +242,24 @@ def with_deferred_parent_expiration
Thread.current[:idc_parent_records_for_cache_expiry].clear
end

def with_deferred_attribute_expiration
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth creating a joint method alongside the parent expiry deferral? From the public API's perspective, it's a convenience method and I imagine most clients will want to do both. For our use case we do and it would save us having to nest one within the other!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be nice to collapse those two.

Maybe with_deferred_expiration that calls both with_deferred_parent_expiration and with_deferred_attribute_expiration.

Or with_deferred_expiration(parent: true, attribute: false as a way to control that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I am going to merge the two methods, and test again. 👍

test/test_helper.rb Outdated Show resolved Hide resolved
lib/identity_cache/query_api.rb Outdated Show resolved Hide resolved
lib/identity_cache/cache_fetcher.rb Show resolved Hide resolved
@drinkbeer drinkbeer marked this pull request as ready for review October 7, 2024 15:43
test/test_helper.rb Outdated Show resolved Hide resolved
@@ -240,6 +242,24 @@ def with_deferred_parent_expiration
Thread.current[:idc_parent_records_for_cache_expiry].clear
end

def with_deferred_attribute_expiration
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be nice to collapse those two.

Maybe with_deferred_expiration that calls both with_deferred_parent_expiration and with_deferred_attribute_expiration.

Or with_deferred_expiration(parent: true, attribute: false as a way to control that.

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.

4 participants