[
https://issues.apache.org/jira/browse/YUNIKORN-1403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17734461#comment-17734461
]
Simon Blessenohl commented on YUNIKORN-1403:
--------------------------------------------
Rewriting the code as you suggested makes sense to me. Just one small thing: as
far as I can see, the condition you proposed would log an overflow for (e.g.)
used = 0 and available = 1000 because (usedResource > 0) == (availableResource
> 0) is false and (absResValue < 0) is also false. But we do not want to log an
overflow in this case. Thus, I wondered whether we should use usedResource >= 0
rather than usedResource > 0?
{code:java}
if ((usedResource >= 0) == (availableResource > 0)) == (absResValue < 0) ||
(div > math.MaxInt64) || (div < math.MinInt64) {{code}
If you're okay with that, I would change the code to use that condition and
adapt my MR accordingly.
Concerning the tests: since we don't clamp the absResValue in case of an
overflow to math.MaxInt64 or math.MinInt64 anymore, the [current
overflow-provoking
tests|https://github.com/apache/yunikorn-core/blob/a6a90bb0408a3004592f06d8a79335ac326dcfe4/pkg/common/resources/resources_test.go#L1507]
would fail. We could change the assertions in those tests to "the return value
must equal `int64(float64(used) * 100 / float64(capacity))`, whatever that
evaluates to on the architecture on which the tests run". But if we say that
the return value for such inputs "is not really correct" / meaningful anyways,
then we probably shouldn't write test that ensure that specific values are
returned. Instead, my hunch would be to call CalculateAbsUsedCapacity with
overflow-provoking inputs in the tests but don't make any assertions, just to
ensure that the function does not panic in such cases. Does that sound
reasonable?
> CalculateAbsUsedCapacity: overflow unit tests
> ---------------------------------------------
>
> Key: YUNIKORN-1403
> URL: https://issues.apache.org/jira/browse/YUNIKORN-1403
> Project: Apache YuniKorn
> Issue Type: Improvement
> Components: core - common, test - unit
> Reporter: Wilfred Spiegelenburg
> Assignee: Simon Blessenohl
> Priority: Major
> Labels: newbie, pull-request-available
>
> CalculateAbsUsedCapacity is guarded against positive and negative overflow.
> The postive case is unit tested. The negative case is not unit tested.
> Need to add a test case for te negative side:
> {code}
> // protect against negative integer overflow
> if absResValue > 0 && div < 0 {
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
