Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Change return value of NodeOrderFn from int to float #708

Closed
lmzqwer2 opened this issue Apr 4, 2019 · 8 comments · Fixed by #719
Closed

Change return value of NodeOrderFn from int to float #708

lmzqwer2 opened this issue Apr 4, 2019 · 8 comments · Fixed by #719

Comments

@lmzqwer2
Copy link
Contributor

lmzqwer2 commented Apr 4, 2019

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:
definition of NodeOrderFn is

type NodeOrderFn func(*TaskInfo, *NodeInfo) (int, error)

but the int type would lost precision when two node is similar.

What you expected to happen:
we can distinguish two node with similar priority

How to reproduce it (as minimally and precisely as possible):
I wanna to change the type of return value from int to float

type NodeOrderFn func(*TaskInfo, *NodeInfo) (float64, error)

Anything else we need to know?:

Environment:

  • Kubernetes version (use kubectl version): N/A
  • Cloud provider or hardware configuration:N/A
  • OS (e.g. from /etc/os-release):N/A
  • Kernel (e.g. uname -a):N/A
  • Install tools:N/A
  • Others:N/A
@hex108
Copy link
Contributor

hex108 commented Apr 5, 2019

hi @lmzqwer2 , where does it lose precision in the function NodeOrderFn ?

@k82cn
Copy link
Contributor

k82cn commented Apr 8, 2019

where does it lose precision in the function NodeOrderFn ?

I think he means the case 3.1 vs. 3.2 :)

@lmzqwer2
Copy link
Contributor Author

lmzqwer2 commented Apr 8, 2019

where does it lose precision in the function NodeOrderFn ?

I think he means the case 3.1 vs. 3.2 :)

Yes, it is the case. The int type would treat 3.1 vs 3.2 as 3 vs 3 or 4 vs 4, and then we could not choose the best fit one.

@hex108
Copy link
Contributor

hex108 commented Apr 8, 2019

Sorry, I could not get the point. Every score that calculated at each step in nodeOrderFn is int, where is 3.1 or 3.2 calculated?

@lmzqwer2
Copy link
Contributor Author

lmzqwer2 commented Apr 8, 2019

Sorry, I could not get the point. Every score that calculated at each step in nodeOrderFn is int, where is 3.1 or 3.2 calculated?

In nodeorder function named leastReq, the priority calculated as

((capacity - requested) * int64(schedulerapi.MaxPriority)) / capacity

the result of this priority should be naturally float ((4Gi - 1.5Gi) * 10 / 4Gi = 6.25 ) but now is int64 ( = 6) because of the integer division. The precision is lost in the first place.

In this case, it is imprecise.

So, I provide a way to keep the priority value precise, and all the lost of precision would be just the behavior of strategy itself (like the leastReq using integer division).

@thandayuthapani
Copy link
Contributor

@lmzqwer2 Yes, But score returned by leastRequestedFn is int. So it does make any change in precision if we change return type from int to float, because it is not going to make any difference.

@k82cn
Copy link
Contributor

k82cn commented Apr 8, 2019

we're going to add new nodeOrderFn later which dependent on this RP.

@lmzqwer2
Copy link
Contributor Author

lmzqwer2 commented Apr 8, 2019

@lmzqwer2 Yes, But score returned by leastRequestedFn is int. So it does make any change in precision if we change return type from int to float, because it is not going to make any difference.

The old functions like leastRequestedFn should modify carefully, so I keep all of its behavior unchanged now.

But new function could benefit from it immediately.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants