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

Enhance Timer/Cache/CachedValues API #399

Merged
merged 14 commits into from
Sep 16, 2024
Merged

Enhance Timer/Cache/CachedValues API #399

merged 14 commits into from
Sep 16, 2024

Conversation

lbwexler
Copy link
Member

@lbwexler lbwexler commented Sep 11, 2024

  • New BaseService factory methods + Docs

  • Require/Check unique names for *all *service resources

  • Use NamedVariant for Timer

  • Improvements to Timer to avoid extra executions when primary instance changes

  • Unrelated cleanups to Timer declarations and deprecate onTimer

  • Logs should get archived on non-primary server as well

See also change on toolbox

@TomTirapani
Copy link
Member

TomTirapani commented Sep 13, 2024

Looks good to me!

My main concern is that the timer having finished on primary does not necessarily mean that the results of that timer have been replicated and that it doesn't need to run on the new primary... thinking about very large datasets where it could potentially be brought down in the middle of serialisation, leaving the new primary with no data. But that seems like an edge case that maybe we don't need to worry about

@lbwexler
Copy link
Member Author

lbwexler commented Sep 13, 2024

Tom -- that's a really good and subtle point -- had not thought about that one. Seems like the main concern there is indeed about guarantees about "initialization". (e.g. if a cluster job were to just miss a cycle doing refresh work, that does not seem like a huge problem)

This concern would be mitigated by the change greg and I have been talking about which is having all nodes block their initialization until the primary node has indicated that it completed its initialization. I suppose there would still be some risk that the initialization didn't actually complete, if we have no way of knowing when it is fully sent.

Truly hardening some of this might require HZ "cp subsystem" that we don't have access to because it is in the enterprise product.

CHANGELOG.md Show resolved Hide resolved
+ Use NamedVariant for Timer
+ Require/Check unique names for all service resouces
+ Logs should get archived on non-primary server.
@lbwexler lbwexler changed the title Enhance timer Enhance Timer/Cache/CachedValues API Sep 14, 2024
Copy link
Member

@amcclain amcclain left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Two follow-up questions - small one on if we should check useCluster and bigger one on naming of replicated obj getters (or rather, creators)

@lbwexler lbwexler merged commit 675bfc3 into develop Sep 16, 2024
4 checks passed
@lbwexler lbwexler deleted the enhanceTimer branch September 16, 2024 21:51
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.

3 participants