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

feat: cross-node cache warming #1875

Closed
wants to merge 3 commits into from
Closed

Conversation

Ziinc
Copy link
Contributor

@Ziinc Ziinc commented Dec 4, 2023

This PR adds cross-node cache warming to Cachex caches.

  • persistent node selection, to prevent imports from different nodes
  • warm on cache startup in non-blocking way
  • stream batches to CacheWarmer genserver, avoiding costly full table export.

Possible future extensions:

  • add to health check, to prevent request hits before cache is fully warmed.
  • Source.Supervisor boot should wait for cache warming to be complete.

@Ziinc Ziinc marked this pull request as ready for review December 4, 2023 07:57
@Ziinc Ziinc requested a review from chasers December 4, 2023 07:57
@Ziinc Ziinc force-pushed the feat/cross-node-cache-warming branch from 0670527 to 4825040 Compare December 4, 2023 08:25
@chasers
Copy link
Contributor

chasers commented Dec 4, 2023

Commenting for comms but lets wait on this one. Lots of edge cases to consider.

end
end)
|> Enum.filter(&(&1 != nil))
|> Enum.sort_by(&Atom.to_string/1)
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC Atom.to_string call is not needed, as it is default behaviour of comparing atoms.

end)
|> Enum.filter(&(&1 != nil))
|> Enum.sort_by(&Atom.to_string/1)
|> List.first()
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorting and then fetching first is the same as finding minimum.

Comment on lines +38 to +40
|> Enum.filter(&(&1 != nil))
|> Enum.sort_by(&Atom.to_string/1)
|> List.first()
Copy link
Contributor

@hauleth hauleth Jan 30, 2024

Choose a reason for hiding this comment

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

Suggested change
|> Enum.filter(&(&1 != nil))
|> Enum.sort_by(&Atom.to_string/1)
|> List.first()
|> Enum.filter(&(&1 != nil))
|> Enum.min(&<=/2, fn -> nil end)

@Ziinc
Copy link
Contributor Author

Ziinc commented Mar 29, 2024

Closing this, as:

  1. db upgrades as allowed us to spin up multiple nodes at concurrently now. This makes this PR obsolete as it is built on the assumption of sequential node startups.
  2. a more scalable and safer way would be to broadcast db data fetching across the cluster, instead of sharing cached values.
  3. caches should pre-warm themselves

@Ziinc Ziinc closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants