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

caching-resolvers: add cache reloader and reloading service #48

Merged
merged 6 commits into from
May 5, 2023

Conversation

yaauie
Copy link
Member

@yaauie yaauie commented Apr 22, 2023

extends cache TTL for both hits and misses to 24 hours, and adds a pair of reloading services to allow cached entries to be updated as frequently as once per minute.

This ensures changes are picked up quickly, while allowing this plugin to survive a period of ES downtime by using possibly-stale values.

Because reloading is also done out-of-band, the event processor no longer is at risk of pausing to load a pipeline that hadn't changed simply because our cache ttl had expired.

extends cache TTL for both hits and misses to 24 hours, and adds a
pair of reloading services to allow cached entries to be updated as
frequently as once per minute.

This ensures changes are picked up quickly, while allowing this plugin
to survive a period of ES downtime by using possibly-stale values.
@jsvd jsvd self-requested a review April 24, 2023 09:40
// start reload services for our resolvers
final ArrayList<Service> services = new ArrayList<>();
eventToPipelineNameResolver.innerCacheReloader().ifPresent(cacheReloader -> {
final AbstractScheduledService.Scheduler pipelineNameReloadSchedule = newFixedRateSchedule(Duration.ofSeconds(60), Duration.ofSeconds(60));
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here wrt abstracting Duration.ofSeconds(60).

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly, DRYing this to a constant would be trivial.

The AbstractScheduledService#newFixedSchedule differentiates between initial-delay and frequency, so there isn't really a way to simplify this interface.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

Overall the code works as described. Tested multiple scenarios of hits/misses, failure to reach ES, reconnection, and pipeline updates.

Added a few nitpicks tweaking logs to improve their usefulness.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM w/ a minor nitpick on doc wording.

docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
@yaauie yaauie merged commit c445e93 into elastic:main May 5, 2023
@yaauie yaauie deleted the resolver-reloading branch May 5, 2023 19:19
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.

2 participants