-
Notifications
You must be signed in to change notification settings - Fork 20.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
les: move client pool to les/vflux/server #22495
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
323cbf4
les: move client pool to les/vflux/server
zsfelfoldi 5409e4c
les/vflux/server: un-expose NodeBalance, remove unused fn, fix bugs
zsfelfoldi 42f5b60
tests/fuzzers/vflux: add ClientPool fuzzer
zsfelfoldi e84b252
les/vflux/server: fixed balance tests
zsfelfoldi cda86d9
les: rebase fix
zsfelfoldi c77a70b
les/vflux/server: fixed more bugs
zsfelfoldi 89c45fa
les/vflux/server: unexported NodeStateMachine fields and flags
zsfelfoldi ed69bfb
les/vflux/server: unexport all internal components and functions
zsfelfoldi f8700b5
les/vflux/server: fixed priorityPool test
zsfelfoldi fc9790e
les/vflux/server: polish balance
rjl493456442 b63f123
les/vflux/server: fixed mutex locking error
zsfelfoldi 42d08dc
les/vflux/server: priorityPool bug fixed
zsfelfoldi 23ab832
common/prque: make Prque wrap-around priority handling optional
zsfelfoldi 31dd954
les/vflux/server: rename funcs, small optimizations
zsfelfoldi 9332261
les/vflux/server: fixed timeUntil
zsfelfoldi 380d626
les/vflux/server: separated balance.posValue and negValue
zsfelfoldi b914785
les/vflux/server: polish setup
rjl493456442 0db4ff4
les/vflux/server: enforce capacity curve monotonicity
zsfelfoldi 7032e9d
les/vflux/server: simplified requestCapacity
zsfelfoldi f7b4531
les/vflux/server: requestCapacity with target range, no iterations in…
zsfelfoldi e294fff
les/vflux/server: minor changes
zsfelfoldi 50ddf0e
les/vflux/server: moved default factors to balanceTracker
zsfelfoldi f1000d6
les/vflux/server: set inactiveFlag in priorityPool
zsfelfoldi 4a56ebe
les/vflux/server: moved related metrics to vfs package
zsfelfoldi 590a834
les/vflux/client: make priorityPool temp state logic cleaner
zsfelfoldi 46a3a27
les/vflux/server: changed log.Crit to log.Error
zsfelfoldi aca9e3a
add vflux fuzzer to oss-fuzz
zsfelfoldi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between the
a>b
anda-b>0
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way they handle numeric overflows. For example,
MaxInt64 > MinInt64
butMaxInt64-MinInt64 < 0
. If we use the latter type of comparison then the individual priority values are allowed to overflow and "wrap around" the 64 bit range many times as long as the biggest difference between two priorities in the queue at any moment can be expressed in theint64
range.flowcontrol.ClientManager
is a good example for this mode where there is a cumulative "buffer recharge integrator" that is continuously being increased and does overflow.rcQueue
item priorities are all calculated relative to this value and the difference between any two item priorities fits safely intoint64
. The first (free clients only) client pool design also did take advantage of this feature but the current priority pool scheme uses absolute values.This "wrap around" feature requires care to use and until now all
prque.Prque
-s operated in this mode. Now it's only used if the queue is explicitly created with theNewWrapAround
constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks dangerous to me. Firstly it introduces this trick into
stack
,prque
very implicitly. Besides are we sure thatflowcontrol.ClientManager
can handle the overflow properly?It's indeed increased all the time. But when it's overflow, all the relative calculations will be affected.
For example
This value can be a large negative value if the
rcqNode.rcFullIntValue
is overflowed.Then the
lastUpdate
will also be the large negative.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should fix the overflow in the flowcontrol manager instead of introducing the tricks into the common data structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR reverts to the non-tricky version in most of the cases and does not change the behavior of the client manager. I will change client manager's priority calculation and get rid of thip wrap-around trick altogether in a next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!