Allow hard reset of a DSCP pool - #50
Conversation
Will support worker pool size changes - but in a disruptive manner
| -define(DSCP_POOLS, [be_pool,af4_pool,af3_pool,af2_pool,af1_pool]). | ||
|
|
||
| % @doc | ||
| % A hard reset of a dscp pool is tested only on a cluster not running active |
There was a problem hiding this comment.
This is a bit worrying. There will be a day that someone forgets to kill a query and then we are in untested land...
The solutions that I can see are:
- Test and know what happens, reduce negative effects
- Have this reset function kill all queries before doing anything else. Which kind of seems to be happening (document that this is what is going to happen).
- disallow the function when queries are active
| ok|{error, unexpected_state}. | ||
| hard_reset_dscp_pool(AF1, AF2, AF3, AF4, BE) -> | ||
| {PoolList, PoolMap} = | ||
| element(4, sys:get_state(riak_core_node_worker_pool_sup)), |
There was a problem hiding this comment.
At least in later OTPs this function is not clearly defined for the supervisor behaviour?? Is this use of undocumented feature?
https://www.erlang.org/doc/apps/stdlib/sys.html#get_state/2
Assumingly it's correct, but a bit of a pity to have to rely upon use of internal data structure.
|
|
||
| -define(DSCP_POOLS, [be_pool,af4_pool,af3_pool,af2_pool,af1_pool]). | ||
|
|
||
| % @doc |
There was a problem hiding this comment.
Use %% @doc for compatibility with edoc parser
| end | ||
| end, | ||
| 1, | ||
| 10 |
There was a problem hiding this comment.
10 ms is really a short upper limit. Sure you don't need something slightly longer for a busy production machine? It's an upper limit, so you will possibly not get there in most cases, but if very busy you want to restart anyway, right?
ThomasArts
left a comment
ThomasArts
left a comment
There was a problem hiding this comment.
Can we not just kill the process riak_core_node_worker_pool_sup and let the logic in it supervisor riak_core_sup restart it?
| lists:foreach( | ||
| fun({PoolName, PoolSize}) -> | ||
| riak_core_node_worker_pool_sup:start_pool( | ||
| riak_kv_worker, |
There was a problem hiding this comment.
Not nice to have a hard-coded dependency on riak_kv here
|
I'm going to close this PR, as I think the review comments are valid. If it should exist, it should be in riak_kv. It is a helper function to do something untested, to avoid the need for the operator to achieve this through their own scripts ... however it is not future proof (e.g. the use of sys:get_state) so it stores up problems for the future. In the long term the issue needs to be resolve about ensuring the loss of worker is detected in processes that rely on workers, to allow this to be done in a valid way. Rushing this patch into 3.4.1 is too risky. |
2 participants
Will support worker pool size changes - but in a disruptive manner.
As there are potential problems caused by resetting a pool - e.g. the impact on tree rebuilds, it is not anticipated to be an exposed/documented option.