-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Track EWMA[1] of task execution time in search threadpool executor #24989
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
Conversation
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.
Throw out -1s?
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.
Assert that >= 0 unless t != null?
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.
Even if t weren't null, the time should be >= 0; I'll add an assert
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 might be nice to have a test that starts with 0 and continually adds the same number and asserts that eventually the average comes near enough to the number. Or something that asserts that the changes get smaller with additional data points.
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.
Sure, I've added this test, thanks for the suggestion
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.
I'm curious about why you chose this. If you don't have a good reason for that number then maybe leave a comment about how this is just a guess.
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's totally a random pick, I'll add a comment. I'm planning on revisiting the number once all the parts are in place (so I can test different values)
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.
Probably worth a comment about why the target is a good starting point.
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.
Good idea, I'll add that
|
Thanks for the comments @nik9000, I pushed commits addressing them! |
7faa88d to
1685c38
Compare
This is the first step towards adaptive replica selection (elastic#24915). This PR tracks the execution time, also known as the "service time" of a task in the threadpool. The `QueueResizingEsThreadPoolExecutor` then stores a moving average of these task times which can be retrieved from the executor. Currently there is no functionality using the EWMA yet (other than tests), this is only a bite-sized building block so that it's easier to review. [1]: EWMA = Exponentially Weighted Moving Average
1685c38 to
b6a2b8d
Compare
This is the first step towards adaptive replica selection (#24915). This PR
tracks the execution time, also known as the "service time" of a task in the
threadpool. The
QueueResizingEsThreadPoolExecutorthen stores a moving averageof these task times which can be retrieved from the executor.
Currently there is no functionality using the EWMA yet (other than tests), this
is only a bite-sized building block so that it's easier to review.
[1]: EWMA = Exponentially Weighted Moving Average