Add protection against folsom stat errors #356

Merged
merged 1 commit into from Aug 20, 2013

Conversation

Projects
None yet
2 participants
Contributor

engelsanchez commented Aug 15, 2013

Folsom may sometimes return an error tuple if something goes wrong (see
folsom_ets.erl), but our code was only catching exceptions. So the error
would end up being used as a valid value and crash the riak_kv_stat
process later. This fixes that problem and gives us better protection
from folsom funkiness.

src/riak_core_stat_q.erl
@@ -90,6 +90,9 @@ get_stat(Stat) ->
Pid = riak_core_stat_calc_sup:calc_proc(Stat),
riak_core_stat_calc_proc:value(Pid).
+err2unavailable({error, _, _}) -> unavailable;
@evanmcc

evanmcc Aug 19, 2013

Contributor

is there a reason that we're not logging what the folsom error actually is here? would it make sense to just throw an exception so that it gets dumped into the normal, logged error path in the catch below?

@engelsanchez

engelsanchez Aug 19, 2013

Contributor

Yes, it makes sense to change it so that all errors go through the same path. I'll update it soon.

Add protection against folsom stat errors
Folsom may sometimes return an error tuple if something goes wrong (see
folsom_ets.erl), but our code was only catching exceptions. So the error
would end up being used as a valid value and crash the riak_kv_stat
process later. This fixes that problem and gives us better protection
from folsom funkiness.
Contributor

engelsanchez commented Aug 19, 2013

Changed to throw folsom error tuples, so all errors go down the same code path. The astute reader might notice a rebase...

@@ -90,6 +90,10 @@ get_stat(Stat) ->
Pid = riak_core_stat_calc_sup:calc_proc(Stat),
riak_core_stat_calc_proc:value(Pid).
+throw_folsom_error({error, _, _} = Err) ->
@evanmcc

evanmcc Aug 19, 2013

Contributor

in an ideal world, I feel that this would be called maybe_throw_folsom_error, but that is deep in the nit-picky weeds.

@engelsanchez

engelsanchez Aug 19, 2013

Contributor

Well, if it's a folsom error, it will absolutely throw it. Not maybe. That's my argument and I'm sticking to it :)

Contributor

evanmcc commented Aug 19, 2013

👍, all folsom errors hit the logging path now, neither crash kills stats, everything seems happy.

engelsanchez added a commit that referenced this pull request Aug 20, 2013

@engelsanchez engelsanchez merged commit 41add84 into 1.4 Aug 20, 2013

1 check failed

default The Travis CI build failed
Details

@engelsanchez engelsanchez deleted the eas-folsom-stat-error-protection branch Aug 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment