Also Add a unit test to confirm it doesnt overflow for valid conditions ?

I thought about that but I guess that should be part of other test that calls the checkOverFlow function. I don't have strong opinion on this.

definetly should have a test confirming it doesnt always overflow. that would be pretty bad :)

+1

ack..

• is this redundant to the weight check ? After all if totalPriority overflows, then totalPriority+ weight overflows as well in all cases, right ?

Once it overflows, no way it can exceed, so == should be fine.

but why check == if we're planning on checking below?

So, by the time the function is called, one of the variables could have already at MaxInt and after that there is no point in doing an addition as it would throw a buffer overflow, so its kind of

• Failing early.
• The computation wouldn't cause an error.

Youre right regarding >=, ill delete that to not confuse people :) .

final thought is that IMO if this whole function can be replaced with return (total+weight)-weight != 0, it shouldn't be a function at all, b/c one line of code (excluding old school perl code golf examples) is usually better then 5.

will let you guys decide though...

weight*10 is being checked below, right?

Right but we need to have check for * but that's for the calling function.(Means if we have a unit test for getPriorityFunctionConfigs.)

call a total, and call b weight...

Makes sense. The only thought that I had was, this could be used anywhere, so did not name variables to reflect weight and total.

actually just delete a/b we dont need them do we

+1

Ok..

so this is just return totalPriority+weight-totalPriority !=weight i think thats pretty much always true... ? in this universe ...

Right but for readability..

gotcha, so its overflow i see whats going on here now

just make it oneline, and add a comment about how in go we have to test for int overflow additions.

gotcha makes sense now

i'd prefer

return (totalPri + weight)-totalPriority != weight
``` since that makes it obvious that your testing the absurd boundary overflow.  but i guess just an opinion. 

IMO we should call this function checkIntegerOverflow

ok, makes sense

checkPriorityOverflow ?

I guess, if we want to use this in someother location, its better to generalize the name to checkIntegerOverflow as this function can check for any 2 int's overflow.

if so, I'd like to move it to util and export it.

or if the check is simple enough, this func is not necessary. But we need to add unit test for priority config check.

The check may be simple enough, my only concern is I have to write unit tests for getPriorityFunctionConfigs, which is kind of orthogonal to issue. IMO, the best thing to do is leave this function as is here now and move it to utils once we find other functions that can use this. I am open to other suggestions too.

In Go, I'm not sure whether sum is still a valid value after overflow; in C, the sum maybe randoms after overflow (e.g. checking by interrupt or others) in some arch.

So I'd prefer to

return weight >= MAX_INT || totalPriority >= MAX_INT || weight >= MAX_INT - totalPriority

or just weight > MAX_INT - totalPriority

(when weight will > MAX_INT?)

(when weight will > MAX_INT?)

Thats pretty hypothetical case but this logic in general seems concise. I will use this. Thanks @k82cn. Just to be clear, since weight is always +ve.

I'd like make 10 as MaxPriority.

can we externalized these or add MaxUint and MaxInt to math.MaxInt math.MaxUInt ? I'm pretty sure we can, and If not, we should add them to a util somewhere.

lets accomplish this TODO in this PR, ... just add a third arg to your overflow function below, so that it checks everything for us.....

if totalPriority, err := addPrioritySafely(totalPriority, config.Weight, MaxPriority); err != nil {
   return nil, fmt.Errorf("Total priority of priority functions has overflown %v", err)
}

Agree that we should handle overflow for weight*MaxPriority instead of TODO.

And I'd suggest to move this check to plugin/pkg/scheduler/api/validation/validation.go; as it also check weight against negative number.

haha this simple PR is getting pretty interesting :). Yeah, k82cn's idea is reasonable, if we can move this thing into a generic place (make sure to create corresponding unit test) thats great.

ok, sorry, now that I better understand whats going on here, im going to suggest you call this addPrioritySafely ,

• Check weight*Maxpriority multiplies w/o overflow.
• then check that totalPriority increments w/o overflow.
• Return error if fails.
• Otherwise return the new added on value of totalPriority

That will be super easy to understand. Sorry about changing my mind :).

@jayunit100 , @k82cn, I am fine with making those changes but AFAIU, it seems weights can never overflow, even if it overflows, shouldn't we check them in functions where they were getting updated?

I was just suggesting that the entire addPriority... operation should handle the TODO as well as the other stuff all in one place, and return a new value representing the result of the operation. That makes it really easy to read, unit test, and so on.

I agree with you but the overflow can happen at other places as well where weight is getting updated. So, I was suggesting that we should have checks there instead of here, as this is the last step in calculation. (I meant, we need to fail earlier than this.).

prefer to priority.Weight > schedulerapi.MaxWeight, it'll align with checking against MaxTotalPriority.