Mas i1804 peerdiscovery #1812
Mas i1804 peerdiscovery #1812
Conversation
Refactoring of riak_kv_replrtq to allow sharing of code and interaction between snk and peer
Adds some further logging. Also corrects the comparison between current and discovered peers to avoid unnecessary resets.
Adds operator riak_client functions
src/riak_kv_replrtq_peer.erl
Outdated
| %% Prompt for the discovery of peers | ||
| -spec update_discovery(riak_kv_replrtq_snk:queue_name()) -> boolean(). | ||
| update_discovery(QueueName) -> | ||
| gen_server:call(?MODULE, {update_discovery, QueueName}, 60 * 1000). |
There was a problem hiding this comment.
Do you really want 60 hardcoded here or rather use the macro ?DISCOVERY_TIMEOUT_SECONDS.
src/riak_kv_replrtq_peer.erl
Outdated
| fun({QueueName, _PeerInfo}) -> | ||
| erlang:send_after(MinDelay * 1000, | ||
| self(), | ||
| {prompt_discovery, QueueName}) |
There was a problem hiding this comment.
It takes a few moments to realize that there are 2 interfaces for prompt discovery where this one goes via handle_info and has only one argument that does a lookup for the PeerInfo at the time it executes, whereas the cast version gets the PeerInfo in the interface.
Probably one should comment this difference more clearly. In particular why one does not need to do a state lookup for the cast version
src/riak_kv_replrtq_peer.erl
Outdated
| handle_info({prompt_discovery, QueueName}, State) -> | ||
| {QueueName, PeerInfo} = | ||
| lists:keyfind(QueueName, 1, State#state.discovery_peers), | ||
| ok = prompt_discovery({QueueName, PeerInfo}), |
There was a problem hiding this comment.
I kind of dislike this... you leave the context of the genserver in the cast to later return to it... I wonder if it would not be better to actually call prompt_discovery(QueueName, PeerInfo, regular) at this point.
| riak_kv_replrtq_snk:current_peers(QueueName))) | ||
| end, | ||
| case discover_peers(PeerInfo, StartDelayMS) of | ||
| CurrentPeers -> |
There was a problem hiding this comment.
I guess this is by design, but if you run with Type count_change then CurrentPeers is the empty list. So in case discover_peers returns an empty list, you don't know in which of the two cases you are.
Both cases return false, but the side effects are different.
There was a problem hiding this comment.
Tried to avoid this confusion by not relying on the empty list mis-match - rather a specific mis-match between list and atom. Commented as well. Don't think it is super-clean still, but improved maybe.
src/riak_client.erl
Outdated
| non_neg_integer()) -> list(node()). | ||
| replrtq_reset_all_workercounts(WorkerC, PerPeerL) -> | ||
| UpNodes = riak_core_node_watcher:nodes(riak_kv), | ||
| lists:foldl(replrtq_resetcount_fun(WorkerC, PerPeerL), [], UpNodes). |
There was a problem hiding this comment.
Personally I find the code clearer if you inline this function.
| @@ -409,13 +428,45 @@ handle_info({prompt_requeue, WorkItem}, State) -> | |||
| requeue_work(WorkItem), | |||
| {noreply, State}. | |||
|
|
|||
| terminate(_Reason, _State) -> | |||
| terminate(_Reason, State) -> | |||
There was a problem hiding this comment.
This terminate function may now take considerably more time.
If the supervisor terminates this server, how likely is it that this takes too long compared to the time you want it restarted?
| {SnkWorkerCount, PerPeerLimit}. | ||
|
|
||
| set_worker_counts(SnkWorkerCount, PerPeerLimit) -> | ||
| application:set_env(riak_kv, replrtq_sinkworkers, SnkWorkerCount), |
There was a problem hiding this comment.
Why using app_helper for getting and application for setting? Is this an artefcat of OTP18 or so?
There was a problem hiding this comment.
Never thought about this much, just following inline with its use elsewhere. Looking at the docs application:get_env/3 didn't exist until R16B ... and Riak was initially written prior to that ... so I guess it is just a legacy of this.
It might be one thing to add to the list for the OTP 24+ version of Riak, to refactor throughout and to take this out.
Rename _discovery functions to reduce confusion over multiple things called prompt_discovery doing different things
Saves some pain with pattern matching {ok, V}|undefined
See basho#1804 The heart of the problem is how to avoid needing configuration changes on sink clusters when source clusters are bing changed. This allows for new nodes to be discovered automatically, from configured nodes. Default behaviour is to always fallback to configured behaviour. Worker Counts and Per Peer Limits need to be set based on an understanding of whether this will be enabled. Although, if per peer limit is left to default, the consequence will be the worker count will be evenly distributed (independently by each node). Note, if Worker Count mod (Src Node Count) =/= 0 - then there will be no balancing of the excess workers across the sink nodes. # Conflicts: # rebar.config # src/riak_kv_replrtq_peer.erl # src/riak_kv_replrtq_snk.erl # src/riak_kv_replrtq_src.erl
See #1804
The heart of the problem is how to avoid needing configuration changes on sink clusters when source clusters are bing changed. This allows for new nodes to be discovered automatically, from configured nodes.
Default behaviour is to always fallback to configured behaviour.
Worker Counts and Per Peer Limits need to be set based on an understanding of whether this will be enabled. Although, if per peer limit is left to default, the consequence will be the worker count will be evenly distributed (independently by each node). Note, if Worker Count mod (Src Node Count) =/= 0 - then there will be no balancing of the excess workers across the sink nodes.