Skip to content

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.

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