fix bug in average ring buffer and add negative duration protection to scheduler decision maker #5396
Conversation
…ing decision maker
Codecov Report
@@ Coverage Diff @@
## master #5396 +/- ##
==========================================
- Coverage 76.77% 75.78% -0.99%
==========================================
Files 240 240
Lines 14600 14596 -4
Branches 634 672 +38
==========================================
- Hits 11209 11062 -147
- Misses 3391 3534 +143
... and 31 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -32,18 +32,12 @@ object AverageRingBuffer { | |||
| class AverageRingBuffer(private val maxSize: Int) { | |||
| private var elements = Vector.empty[Double] | |||
| private var sum = 0.0 | |||
| private var max = 0.0 | |||
| private var min = 0.0 | |||
|
|
|||
| def nonEmpty: Boolean = elements.nonEmpty | |||
|
|
|||
| def average: Double = { | |||
| val size = elements.size | |||
There was a problem hiding this comment.
the only place this function is called, it does a nonEmpty check first or skips over it. Now debatably that check should also be happening inside this function for sure to prevent the divide by zero; but since it's not an existing issue for the moment I'm going to merge this in to get this fixed.
|
Good catch 👍 |
|
@bdoyle0182 Thank you for handling this. |
|
I realized this commit fixed my annoying issue for a long time as well. |
…ing decision maker (apache#5396) Co-authored-by: Brendan Doyle <[email protected]> (cherry picked from commit de3e0a8)
Description
The average ring buffer has an edge case where discarding the max and min outliers from the average calculation can result in the average being negative due to the max and min not ever being reset even when the max and min value is ultimately discarded from the ongoing buffer. So the way it's set up it keeps the absolute max and absolute min duration for this function that has ever occurred. I just got rid of the max and min outlier discarding for now because I don't think it's adding much value and to appropriately update the max and min would require a performance hit to the ring buffer. I've also added negative duration buffer protection to the scheduling decision maker so that if such a case were introduced again in the future, the scheduling decision maker can more gracefully handle it and still fallback to scheduling containers when there are stale activations rather than thinking the amount of containers to schedule is negative.
Here is a raw example of how things can go wrong.
There is an edge case on the (sum - max - min) / (size - 2) calculation. The max never gets reset even if the element leaves the buffer. So say you have a random activation that takes 60s to run. The normal case is that all other activations take 50ms. If the buffer size is 10, then the sum should = 500 assuming the 60000 activation has left the buffer. however the max value will still be set to 60000.
(500 - 60000 - 50) / (10 - 2) = -7443ms
Related issue and scope
My changes affect the following components
Types of changes
Checklist: