-
Notifications
You must be signed in to change notification settings - Fork 89
Conversation
Some recommendations here:
|
ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1); | ||
// upon JVM termination, wait for the tasks for up to 100ms before exiting the executor service | ||
ScheduledExecutorService executorService = MoreExecutors.getExitingScheduledExecutorService(executor, 100, TimeUnit.MILLISECONDS); | ||
executorService.scheduleWithFixedDelay(() -> { |
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.
My intellij inspections recommend using a method reference instead of a lambda here: this::postDecodeHelper
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.
Done
@Override public void postDecode() { | ||
if (refreshMinutes == 0) { | ||
postDecodeHelper(); | ||
} else { |
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.
I noticed a pretty serious issue here - in the case where refresh is enabled, we only call postDecodeHelper in the scheduled executor service. While it's true that we call it immediately, it's called asynchronously without blocking. We need to block here so that this value filter can't be used until the post decode step has occured at least once. Therefore, we should always call postDecodeHelper in this method, and change the scheduled executor to not refresh immediately.
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.
Done
No description provided.