-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
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.
src/main/java/co/elastic/logstash/filters/elasticintegration/EventProcessorBuilder.java
Outdated
Show resolved
Hide resolved
// 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)); |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
src/main/java/co/elastic/logstash/filters/elasticintegration/resolver/SimpleResolverCache.java
Show resolved
Hide resolved
src/main/java/co/elastic/logstash/filters/elasticintegration/resolver/SimpleResolverCache.java
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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>
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.