Avoid Request Locks With Task Lag #2244
Conversation
There was a problem hiding this comment.
I'm with you on being worried about these pollers getting stuck for too long. What if each poller also kept track of how long it had been since it last ran, and if too long -> grab high priority lock instead. Too long here could be in the 10m + range for things like persisters, maybe less for some of the others
|
|
||
| private void updateLateTasksByRequestId() { | ||
| long now = System.currentTimeMillis(); | ||
| if (now - lastUpdate > 1000L * configuration.getRequestCacheTtl()) { |
There was a problem hiding this comment.
Do we need to care about request cache ttl at all here? Shouldn't this be running on the leader only?
There was a problem hiding this comment.
This was mostly me not wanting to add another configuration option, and thinking that request cache ttl was an appropriate value for this.
| updateLateTasksByRequestId(); | ||
| return lateTasksByRequestId.containsKey(requestId); |
There was a problem hiding this comment.
I'm a bit worried about this being called in a super tight loop. I know it's mostly going to no-op on the if statement. But, does it make sense to be updating this on a thread that is trying to grab a lock vs just doing it in the background? Then we can be sure every check is just a map.containsKey and never has to worry about heavier calculations on an active path
There was a problem hiding this comment.
My original thinking was that a scheduled thread would be overkill for this, but you're right that this is going to be a hot method and that I should add a background thread specifically for this.
|
@ssalinas - I opted to allow low priority lock requests to go through if the request lock isn't currently locked to avoid pollers getting stuck due to task lag that is unrelated to request lock usage. Do you think that I should still force them to become high priority after enough delay, or is this change enough? |
|
Looks good. I think your suggestion about allowing low priority to run when initially unlocked makes sense. I may have missed some convo on this last week — has anything been discussed about Steve's suggestion to have each poller determine priority based on time since last run? Not necessary, just curious. Maybe a good future improvement? |
This PR prevents request locks from being acquired by lower priority possibly long running components while the request has late pending tasks, to allow higher priority components like the offer scheduler to acquire them. It does not preempt lower priority components, so lag for a request may still build up while they're holding the lock.
List of components using the request lock:
Somewhat concerned about this change introducing difficult bugs, because previously lock acquisition would wait for the lock forever instead of short circuiting. Not sure if it would be better to throw an error instead of logging+returning.
cc - @ssalinas, @pschoenfelder