Sort AAE differences before acting upon them via read-repair

[slfritchie]

        %% Sort the keys.  For vnodes that use backends that preserve
        %% lexicographic sort order for BKeys, this is a big
        %% improvement.  For backends that do not, e.g. Bitcask, sorting
        %% by BKey is unlikely to be any worse.  For Riak CS's use
        %% pattern, sorting may have some benefit since block N is
        %% likely to be adjacent on disk to block N+1.

[engelsanchez]

I would be in favor of reducing the number of convenience variations of this function, but that can be done later. Notice that in the documentation at the beginning of the file compare/2 and compare/3 are explicitly named.

[jrwest]

ooh, i just realized. This file is not in riak_kv any more. It has been in core since #652

[jrwest]

another note, more pertinent to the original comment. Adding compare/4 would be useful for simplifying some of the code in hashtree_tree.erl.

[engelsanchez]

BTW I like compare/4. I would keep it and eye the others for cleanup. Again, not pressing now.

[engelsanchez]

I'm done going over the code now. The comments I had were very minor, with the only little issue being the inconsistency of riak_kv_index_hashtree:compare/2 and the handling of the undefined AccFun (just getting rid of compare/2 would do). I'll be testing this tomorrow.

[engelsanchez]

And of course, the other "minor" issue of a file having completely moved out of the repo that Jordan mentioned :)

[slfritchie]

I think I've addressed all of the review comments. Dialyzer appears happy enough: the total number of warnings has dropped, and no new warnings have been added.

[slfritchie]

Oh, oops. So I patched a hashtree.erl that's now a ghost?

[engelsanchez]

@slfritchie yes, we should do that simple hashtree patch in a separate PR in its new home in riak_core and update this. After that and a bit of testing on the final code, assuming it's all good, I should be able to give you a quick +1

[slfritchie]

Engel: basho/riak_core#411

[engelsanchez]

I believe the comments have been addressed. I've verified sorted AAE repairs manually. EUnit tests have only failures that I've observed lately in the develop branch (get_put_monitor_eqc, crdts) but that don't seem to happen in the builder machines. It looks good. 👍 💃

I'm going to push a manual merge that I used for testing that takes care of the moved hashtree files.