Mas i1801 monitorworkerq #979
Merged
Conversation
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
The original commit to make the common riak_core_node_worker_pool behaviour contained rogue tab types that skew code formatting within github. Removing these tabs here.
Currently no way of knowing what the utilisation of different worker pools is, and how often queue events are occurring.
Changed the stats to ones which appear to be intuitively more useful. Now queue time is logged. As there may be a small number of data points only mean and max is tracked. Secondly we log threshold events. On vnode_worker pool, the threshold will be half the limit (as we don't normally expect to get close to the limit). On the node_worker pools, the threshold will be the limit.
At a cost of backwards compatibility. The renamed stats are of minimal use, and in the most case relatively recent additions, for rarely used features.
As well as monitor queue time, monitor work time. As we know our pool sizes threshold breaches can be estimated by looking at the sum of the work time (as we have a count as well as the average) Easier to understand than threshold_events.
Closed
Otherwise average will be misleading
ThomasArts
reviewed
Nov 9, 2021
src/riak_core_worker_pool.erl
Outdated
| @@ -278,3 +291,56 @@ discard_queued_work(Q, Mod) -> | |||
| ok | |||
| end. | |||
|
|
|||
| -spec poolboy_checkin(pid(), pid(), state()) -> {ok, state()}. | |||
| poolboy_checkin(Pool, Worker, State) -> | |||
There was a problem hiding this comment.
It is not wrong, but changing state as a side effect of a little function called makes it harder to follow the code. I kind of like to have state updates very declarative in a small scope.
In this case that would mean that you pass the pool name in as last argument, instead of the state, and return the checkout or an error. Then you do the state update in the function calling this one...
I do see that this may give some code duplication and probably it's fine this way, but it is harder to get an idea of what happens to the state in the main "loop".
ThomasArts
approved these changes
Nov 9, 2021
Manipulate state only in the main loop
Use Monitors[0]/Checkouts[0] everywhere
ThomasArts
approved these changes
Nov 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add monitoring via riak stats to the worker queues. Monitor both queue time, and work time.