don't include cleaning tasks in instance count #1505
Conversation
| c = new Counter(); | ||
| map.put(key, c); | ||
| } | ||
| Counter c = map.computeIfAbsent(key, k -> new Counter()); |
There was a problem hiding this comment.
we are not using the key in order to determine the value here, should use putIfAbsent instead of computeIfAbsent. Same for the one below
There was a problem hiding this comment.
small gotcha for putIfAbsent: turns out putIfAbsent returns the old value (null) so computeIfAbsent is what we want here (returns existing or computed)
There was a problem hiding this comment.
👍 good find, didn't realize that one
| @@ -133,6 +134,10 @@ public void save(SingularityHostState hostState) throws InterruptedException { | |||
| numTasks.incr(pendingTaskId.getRequestId()); | |||
| } | |||
|
|
|||
| for (SingularityTaskId cleaningTaskId : taskManager.getCleanupTaskIds()) { | |||
| numTasks.decr(cleaningTaskId.getRequestId()); | |||
There was a problem hiding this comment.
Do we still want to include cleaning when checking for under provisioned, this will exclude it for both under and over? Trying to make sure we avoid any false alerts if possible
There was a problem hiding this comment.
I believe so. I'm imagining a request that should have 3 instances, but has 2 running and 1 cleaning. Since we're still counting pending tasks, if one task is pending, then the count is correct, otherwise, it's under provisioned. I think counting cleaning tasks may actually have made us miss potential under provisioned requests in the past
There was a problem hiding this comment.
👍 cool, let's give this a go in staging/qa then
|
👍 lets get this into stable |
|
Just verifying the merge conflicts got resolved correctly and will merge this one too 👍 |
Tasks that are cleaning should not be part of the instance count when checking for over provisioned requests
@ssalinas