Correct abortion of MR input-sending logic #408
Conversation
Without this, a process might continue to send inputs to a MapReduce pipe for a long time after the pipe has disappeared. This change should clear up the majority of cases where a log is spammed with messages about "fitting was gone before startup". There may still be N (one from each of the N primaries of a preflist) as the call must fail once before triggering the exit, but it will not continue through a large input list.
To help the process of shutting down the asynchronus mapred input sender, this patch explicitly kills it whenever it knows that it should die. The previous patch made the sender better able to determine that time on its own, but this should improve the situation even more.
|
For reproducing: |
|
I'll have to leave the code review to someone a bit more familiar the riak_kv (although it looks fine to me, I am not sure if there are spots that might have been missed. I'll spend some more time with it tomorrow), even making the repro stress test worse and worse doesn't seem to have any affect (in a good way). My dev cluster never gets quite as loaded, and while there are errors (similar to the old ones), they are much less common and cease as soon as the bad MR spam ceases. I'd like to spend some more time testing it tomorrow, but consider this a provisional testing +1. |
|
Changes look good. Verified that this change fixes the problem of the cleanup not happening in a timely manner resulting in a massive amount of |
As of basho/riak_pipe@276e90c (released in Riak 1.1) using
riak_pipe:destroy/1causes theriak_pipe_builderprocess to exit with reasonnormalinstead of something abnormal. This means that the asynchronous MapReduce input sender spawned inriak_kv_mrc_pipe:send_inputs_async/3no longer receives an exit signal, despite being linked to the builder. So, an input-sender might continue running long after its target pipe has disappeared. This will fill riak logs with messages of the form:This affects MR input types of explicit bucket-keys, and certain kinds of
modfuninputs.Changing
riak_pipe_builder:destroy/1to make the builder exit abnormally does not completely solve this case, because there are other points at which the builder decides to exit, but does so with reason 'normal' to avoid log spam from its supervisor.In addition, not all MapReduce users use
riak_kv_mrc_pipe:send_inputs_async/3; some use:send_inputs/2,3(non-async). These senders also need to notice that the pipe has vanished.This PR takes a two-pronged attack:
riak_kv_mrc_pipe:send_inputs/3now check the return value of their calls toriak_pipe_vnode:queue_work/2. When the pipe has closed, instead of returningok,:queue_workwill return{error, [worker_startup_failed,...]}. This error is raised for the endpoint to handle.riak_kv_wm_mapredandriak_kv_pb_mapredmodules, handling HTTP and PB MapReduce requests, now explicitly kill the async sender process.Messages about "fitting was gone before startup" may still appear in the log, but they should be limited to one input's N-value worth (since each of the primary vnodes in the preflist must fail before the error is raised). In addition to being protection against holes in this strategy, the explicit kill added to the HTTP and PB endpoints should help reduce the number of spam log messages further.
@evanmcc and @jonmeredith should both be interested in this PR.