mimir-operations: Ensure integer auto-scaling thresholds #3512
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
3 times, most recently
from
c278206 to
37b2170
Compare
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
from
37b2170 to
94cdacd
Compare
aknuds1
changed the title
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
from
94cdacd to
6a3deb7
Compare
| ) | ||
| ); | ||
|
|
||
| std.split(std.toString(siToBytesDecimal(str)), '.')[0] |
There was a problem hiding this comment.
If we are using std.toString(std.parseInt(trigger.threshold)) above, do we need this here?
siToBytes could still return a number (preferably an integer, but decimals shouldn't matter if it's easier to ignore it for now). It's KEDA itself that expects a number in a string so we could do the string conversion just at that point.
There was a problem hiding this comment.
If we are using std.toString(std.parseInt(trigger.threshold)) above, do we need this here?
@jhesketh std.parseInt will fail if e.g. it's a string representing a decimal nmber, so it is necessary.
There was a problem hiding this comment.
Can't we guarantee that siToBytesDecimal() always return a number, then here we just use std.ceil(siToBytesDecimal(str))?
There was a problem hiding this comment.
@pracucci aside from the technique for converting to a number, do you prefer rounding up (ceil) to rounding down, since I'm currently doing the latter?
There was a problem hiding this comment.
I converted siToBytesDecimal to return a number, awaiting @pracucci's reply regarding floor vs ceil. I figured rounding down would be OK personally, but not sure what others think.
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
from
6a3deb7 to
128f7cb
Compare
| ) | ||
| ); | ||
|
|
||
| std.split(std.toString(siToBytesDecimal(str)), '.')[0] |
There was a problem hiding this comment.
If we are using std.toString(std.parseInt(trigger.threshold)) above, do we need this here?
siToBytes could still return a number (preferably an integer, but decimals shouldn't matter if it's easier to ignore it for now). It's KEDA itself that expects a number in a string so we could do the string conversion just at that point.
Thanks @bboreham, will do. |
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
from
128f7cb to
c366640
Compare
In Jsonnet logic to generate KEDA auto-scaling thresholds, ensure that thresholds are integers since KEDA requires this. Signed-off-by: Arve Knudsen <[email protected]>
aknuds1
force-pushed
the
chore/autoscaling-int-threshold
branch
from
c366640 to
08a8209
Compare
| @@ -137,21 +139,27 @@ | |||
| }, | |||
| }, | |||
|
|
|||
| local siToBytes(str) = ( | |||
| local siToBytesString(str) = ( | |||
There was a problem hiding this comment.
From a design perspective, I think this should be a generic utility returning a number and then if the call needs a string it will cast it to string (which is what were were doing before).
There was a problem hiding this comment.
@pracucci actually before it could return either a number or a string.
There was a problem hiding this comment.
@pracucci I converted it now into returning an integer.
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
* mimir-operations: Ensure integer auto-scaling thresholds In Jsonnet logic to generate KEDA auto-scaling thresholds, ensure that thresholds are integers since KEDA requires this. Signed-off-by: Arve Knudsen <[email protected]>
What this PR does
Ensure that auto-scaling thresholds in mimir-operations are integers.
Which issue(s) this PR fixes or relates to
We've sometimes made the mistake of setting non-integer KEDA thresholds due to this logic not enforcing the expectation of them being integers.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]