New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add force update flag and rename undefined
version to legacy
#1481
Conversation
Set to `true` to force hashtrees to use version 0 on startup.
Also add entires for each cluster member for quick lookup of exchange completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Please see comments about some cleanup/refactoring I think would be useful to future-us as we need to maintain this code.
[{Index, Version}] -> | ||
Version | ||
end | ||
catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the try/catch here? In case you somehow end up with multiple rows in the ETS table? ets:lookup docs seem to indicate that it should never throw, and given the table is a set (can only have one entry per key) it'd never actually return more than one row. Was there something else you were defending against here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In looking around at how this was implemented in other places, I came across the riak_core_capability:get function which uses this try catch. I also looked at the docs and didn't understand why it was there and also looked in the commit history for capabilties and didn't find anything. Unssure of if I was missing something, I just added the try catch because there was no downside from a logic perspective. With that said, I agree and will remove the try/catch as I havent found a single reason why it's there.
_ -> | ||
State | ||
end. | ||
|
||
-spec check_remote_exchanges(list(), list()) -> complete | incomplete. | ||
check_remote_exchanges([], Acc) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a lists:all call in disguise - if you take the check_remote_exchanges logic and split it into a few functions you end up with something like:
check_all_remote_exchanges_complete(Nodes) ->
lists:all(fun maybe_check_and_record_remote_exchange/1, Nodes).
maybe_check_and_record_remote_exchange(Node) ->
case ets:lookup(?ETS, Node) of
[{Node, true}] ->
true;
_ ->
Result = check_remote_exchange(Node),
ets:insert(?ETS, {Node, Result}),
Result
end.
This has a few advantages:
- Readability - it's clear what you're trying to accomplish.
- Bails on the first "false" answer
- Doesn't have to build/sort a list of results.
- We're trying to encourage the use of functions like lists:all, lists:map, etc. when they make sense, versus hand-rolling recursive functions on our own (or even using a foldl when you really mean map). This generally helps code maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better way thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea - us functional programmers always seem to start with "let's write a recursive list processing function" rather than looking at other options first :)
_ -> | ||
undefined | ||
end | ||
case application:get_env(riak_kv, force_hashtree_upgrade) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to break up this 4-level deep case statement into something more manageable.
Take a look at http://www.gar1t.com/blog/solving-embarrassingly-obvious-problems-in-erlang.html for some suggestions as to how you might go about it. I realize there's tons of this in our codebase, just trying to make sure we don't add more of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to:
%% @doc Determine the version this tree should use at startup. If update atom is in Opts
%% then we immediately use version in capabilities. Otherwise check if capabilities
%% has flipped to a version yet and if it has, check if we've already upgraded by
%% looking for versioned AAE directory.
-spec determine_version(list(), index(), list()) -> version().
determine_version(Root, Index, Opts) ->
case force_upgrade(Opts) of
true ->
get_cap_hash_version();
_ ->
find_version(Root, Index)
end.
-spec force_upgrade(list()) -> boolean().
force_upgrade(Opts) ->
check_upgrade_opts(Opts, check_upgrade_env()).
-spec check_upgrade_opts(lists(), boolean()) -> boolean().
check_upgrade_opts(_Opts, true) ->
true;
check_upgrade_opts(Opts, _) ->
lists:member(upgrade, Opts).
-spec check_upgrade_env() -> boolean().
check_upgrade_env() ->
case application:get_env(riak_kv, force_hashtree_upgrade) of
{ok, true} ->
true;
_ ->
false
end.
-spec get_cap_hash_version() -> version().
get_cap_hash_version() ->
riak_core_capability:get({riak_kv, object_hash_version}, legacy).
-spec find_version(list(), index()) -> version().
find_version(Root, Index) ->
check_root_version(Root, Index, get_cap_hash_version()).
-spec check_root_version(list(), index(), version()) -> version().
check_root_version(Root, Index, Version) when is_integer(Version) ->
case filelib:is_dir(filename:join(filename:join(Root, "v" ++ integer_to_list(V)),integer_to_list(Index))) of
true ->
Version;
false ->
legacy
end;
check_root_version(_Root, _Index, Version) ->
Version.
lists:member(upgrade, Opts). | ||
|
||
-spec check_upgrade_env() -> boolean(). | ||
check_upgrade_env() -> | ||
case application:get_env(riak_kv, force_hashtree_upgrade) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - one more. How about using the 3-arity version of get_env and providing false as the default, rather than having to deal with the {ok, true} - can just match on true and _ at that point (in case someone put something stupid other than true in there).
Rename AAE hashtree version from
undefined
tolegacy
.Add public ETS table to handle partition version requests