Skip to content

Bz808 list keys opt#37

Merged
5 commits merged into
masterfrom
bz808-list-keys-opt
Feb 23, 2011
Merged

Bz808 list keys opt#37
5 commits merged into
masterfrom
bz808-list-keys-opt

Conversation

@slfritchie

Copy link
Copy Markdown
Contributor

Reviewer: anyone. Basic gist: using the first backend API function
that exists.

Add an iterative process to try the three different API variations for
folding over keys in various backends.  IIRC there's a different
bug/change-request to change these APIs to be more harmonious, so we'll
tackle that bridge when we take it by the horns.

Also added more extensive backend unit tests: for bitcask, cache, dets,
ets, fs, and gb_trees backends.  And added new-style folding API to the
ETS backend, as an example (since it doesn't do anything differently than
the fold implementation).
Comment thread src/riak_kv_vnode.erl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to this bitcask case for fold_keys? By my read this will now cause bitcask to fold over all data rather than keys now.

@slfritchie

Copy link
Copy Markdown
Contributor Author

Bleh, my commit missed riak_kv_bitcask_backend.erl. It's pooooshed now.

@jonmeredith

Copy link
Copy Markdown
Contributor

I get this with innostore master (1.0.3) as the backend

([email protected])1> systest:write(8).
2> systest:read(8).
3> systest:listkeys(8).

=ERROR REPORT==== 17-Feb-2011::14:26:39 ===
** State machine <0.301.0> terminating
** Last event in was {riak_vnode_req_v1,
114179815416476790484662877555959610910619729920,
ignore,
{riak_kv_listkeys_req_v2,<<"systest">>,89531250,
<0.1906.0>}}
** When State == active
** Data == {state,114179815416476790484662877555959610910619729920,
riak_kv_vnode,
{state,114179815416476790484662877555959610910619729920,
riak_kv_innostore_backend,
{state,<<"_114179815416476790484662877555959610910619729920">>,
#Port<0.4521>},
{dict,0,16,16,8,80,48,
{[],[],[],[],[],[],[],[],[],[],[],[],[],
[],[],[]},
{{[],[],[],[],[],[],[],[],[],[],[],[],[],
[],[],[]}}},
false},
undefined,none,60000}
** Reason for termination =
** {{badarity,{#Fun<riak_kv_vnode.2.64444309>,[<<0,0,0,6>>,[]]}},
[{innostore,fold_loop,5},
{innostore,fold,4},
{riak_kv_innostore_backend,fold_bucket_keys,3},
{riak_kv_vnode,'-do_list_keys/6-fun-4-',2},
{lists,foldl,3},
{riak_kv_vnode,do_list_keys,6},
{riak_kv_vnode,handle_command,3},
{riak_core_vnode,vnode_command,3}]}

@jonmeredith

Copy link
Copy Markdown
Contributor

My listkeys test function

listkeys(Size) ->
listkeys(1, Size, <<"systest">>).

listkeys(Start, End, Bucket) ->
{ok, C} = riak:local_client(),
Expect = ordsets:from_list([<<V:32/integer>> || V <- lists:seq(Start, End)]),
{ok, Got} = C:list_keys(Bucket),
Got2 = ordsets:from_list(Got),
ordsets:subtract(Expect, Got2) ++ ordsets:subtract(Got2, Expect).

@jonmeredith

Copy link
Copy Markdown
Contributor

it looks like when innostore does innostore:fold_keys, they callback is expected to be fun(Key, Acc) instead of the fun(Key, _Val, Acc) being passed in.

@slfritchie

Copy link
Copy Markdown
Contributor Author

Yup, bad Scott, no cookie for you. ... I found a copy of your
systest.erl, patched it up with listkeys(), and verified that it's
broken before e7a2687 and fixed after it.

c("/Users/fritchie/b/src/systest/systest.erl").
systest:write(100).
systest:read(100).
[] = systest:listkeys(100).

@jonmeredith

Copy link
Copy Markdown
Contributor

The last wrinkle is that riak_kv_multi_backend does not pass through fold_bucket_keys. It works functionally but it means that list keys ends up doing full folds for innostore / bitcask which is suboptimal.

This may be acceptable if we're going to do the backend refactor sooner rather than later... However I'm not sure we can count on it, even if we think we can.

Add unit test for the multi backend, which was missing.  Verify
via redbug that each of the actual backends is having the proper
fold function called: ets -> fold_bucket_keys, gb_trees -> fold.

19> redbug:start({riak_kv_ets_backend, fold_bucket_keys}).
ok
20> riak_kv_vnode:list_buckets_multi_backend_test().
ok

18:41:32 <{erlang,apply,2}> {riak_kv_ets_backend,fold_bucket_keys,
                                [<0.206.0>,'_',#Fun<riak_kv_vnode.2.64444309>,
                                 []]}
quitting: timeout
21> redbug:start({riak_kv_gb_trees_backend, fold_bucket_keys}).
ok
22> riak_kv_vnode:list_buckets_multi_backend_test().
ok
23> riak_kv_vnode:list_buckets_multi_backend_test().
ok
24> redbug:stop().
{stop,[]}
quitting: local_done
25> redbug:start({riak_kv_gb_trees_backend, fold}).
ok
26> riak_kv_vnode:list_buckets_multi_backend_test().
ok

18:42:02 <{erlang,apply,2}> {riak_kv_gb_trees_backend,fold,
                                [{state,<0.237.0>},
                                 #Fun<riak_kv_vnode.2.64444309>,
                                 [<<"f">>]]}
@slfritchie

Copy link
Copy Markdown
Contributor Author

John, see 55b722d. I also added a list keys unit test for the multi
backend, which had been missing up until now.

@jonmeredith

Copy link
Copy Markdown
Contributor

Having different interfaces is so painful. It calls the correct fold_bucket_keys for bitcask, but not for innostore

([email protected])14> systest:listkeys(1, 1000, <<"i">>).
(<0.172.0>) call riak_kv_innostore_backend:fold({state,<<"_365375409332725729550921208179070754913983135744">>,#Port<0.4748>},#Fun<riak_kv_vnode.2.64444309>,[])
(<0.172.0>) returned from riak_kv_innostore_backend:fold/3 -> [<<0,0,3,232>>,

With

     {storage_backend, riak_kv_multi_backend},
        {riak_kv_dets_backend_root, "data/dets"},
        {riak_kv_fs_backend_root, "data/fs"},
        {multi_backend_default, default},
        {multi_backend, [{default, riak_kv_dets_backend, [{riak_kv_dets_backend_root, "data/dets"}]},
                         {bitcask, riak_kv_bitcask_backend, []},
                         {inno,    riak_kv_innostore_backend, []}]},

and

([email protected])1> riak_core_bucket:get_bucket(<<"b">>).
{allow_mult,false},
{backend,bitcask},
{big_vclock,50},
{chash_keyfun,{riak_core_util,chash_std_keyfun}},
{dw,quorum},
{last_write_wins,false},
{linkfun,{modfun,riak_kv_wm_link_walker,mapreduce_linkfun}},
{n_val,3},
{name,<<"b">>},
{old_vclock,86400},
{postcommit,[]},
{precommit,[]},
{r,quorum},
{rw,quorum},
{small_vclock,10},
{w,quorum},
{young_vclock,20}
2> riak_core_bucket:get_bucket(<<"i">>).
[{allow_mult,false},
{backend,inno},
{big_vclock,50},
{chash_keyfun,{riak_core_util,chash_std_keyfun}},
{dw,quorum},
{last_write_wins,false},
{linkfun,{modfun,riak_kv_wm_link_walker,mapreduce_linkfun}},
{n_val,3},
{name,<<"i">>},
{old_vclock,86400},
{postcommit,[]},
{precommit,[]},
{r,quorum},
{rw,quorum},
{small_vclock,10},
{w,quorum},
{young_vclock,20}]

Fix Jon's less-than-optimal bug as pointed out in pull req #37.
Also, since the multi backend is actually bucket aware, we save
work by only querying the one backend that we know is responsible
for the bucket.

Verifying, at console:

{ok, C} = riak:local_client().
C:set_bucket(<<"i">>,[{backend, inno}]).
C:set_bucket(<<"b">>,[{backend, bitcask}]).
[C:put(riak_object:new(<<"i">>, <<X:32>>, <<X:32>>)) || X <- lists:seq(1,5)].
[C:put(riak_object:new(<<"b">>, <<X:32>>, <<X:32>>)) || X <- lists:seq(6,10)].
[C:put(riak_object:new(<<"x">>, <<X:32>>, <<X:32>>)) || X <- lists:seq(11,15)].
[{Bucket, C:list_keys(Bucket)} || Bucket <- [<<"i">>, <<"b">>, <<"x">>, <<"none">>]].

Config additions:

--- rel/riak/etc/app.config	2011-02-17 16:50:36.000000000 -0600
+++ /tmp/app.config	2011-02-22 15:01:55.000000000 -0600
@@ -1,6 +1,11 @@
 %% -*- tab-width: 4;erlang-indent-level: 4;indent-tabs-mode: nil -*-
 %% ex: ts=4 sw=4 et
 [
+{innostore, [
+    {data_home_dir, "/tmp/innodb"}, %% Where data files go
+    {log_group_home_dir, "/tmp/innodb"}, %% Where log files go
+    {buffer_pool_size, 268435456} %% 256MB in-memory buffer in bytes
+]},
  %% Riak Core config
  {riak_core, [
               %% Default location of ringstate
@@ -30,7 +35,15 @@
  {riak_kv, [
             %% Storage_backend specifies the Erlang module defining the
             %storage
             %% mechanism that will be used on this node.
-            {storage_backend, riak_kv_bitcask_backend},
+            %%{storage_backend, riak_kv_bitcask_backend},
+{storage_backend, riak_kv_multi_backend},
+{riak_kv_dets_backend_root, "data/dets"},
+{riak_kv_fs_backend_root, "data/fs"},
+{multi_backend_default, default},
+{multi_backend, [{default, riak_kv_dets_backend,
+    [{riak_kv_dets_backend_root, "data/dets"}]},
+     {bitcask, riak_kv_bitcask_backend, []},
+     {inno,    riak_kv_innostore_backend, []}]},

             %% pb_ip is the IP address that the Riak Protocol Buffers
             %interface
             %% will bind to.  If this is undefined, the interface will not
             %run.

([email protected])1> {ok, C} = riak:local_client().
{ok,{riak_client,'[email protected]',<<1,158,32,8>>}}
([email protected])2> C:set_bucket(<<"i">>,[{backend, inno}]).
ok
([email protected])3> C:set_bucket(<<"b">>,[{backend, bitcask}]).
ok
([email protected])4> [C:put(riak_object:new(<<"i">>, <<X:32>>, <<X:32>>)) ||
X <- lists:seq(1,5)].
[ok,ok,ok,ok,ok]
([email protected])5> [C:put(riak_object:new(<<"b">>, <<X:32>>, <<X:32>>)) ||
X <- lists:seq(6,10)].
[ok,ok,ok,ok,ok]
([email protected])6> [C:put(riak_object:new(<<"x">>, <<X:32>>, <<X:32>>)) ||
X <- lists:seq(11,15)].
[ok,ok,ok,ok,ok]
([email protected])7> [{Bucket, C:list_keys(Bucket)} || Bucket <- [<<"i">>,
<<"b">>, <<"x">>, <<"no">>]].
[{<<"i">>,
  {ok,[<<0,0,0,2>>,
       <<0,0,0,5>>,
       <<0,0,0,3>>,
       <<0,0,0,4>>,
       <<0,0,0,1>>]}},
 {<<"b">>,
  {ok,[<<0,0,0,8>>,
       <<0,0,0,7>>,
       <<0,0,0,9>>,
       <<0,0,0,10>>,
       <<0,0,0,6>>]}},
 {<<"x">>,
  {ok,[<<0,0,0,13>>,
       <<0,0,0,12>>,
       <<0,0,0,11>>,
       <<0,0,0,14>>,
       <<0,0,0,15>>]}},
 {<<"no">>,{ok,[]}}]
@jonmeredith

Copy link
Copy Markdown
Contributor

+1 works for me - merge it!

@sendtopms

Copy link
Copy Markdown

Fantastic. Any write up on this fix, I mean, I would like to see any performance improvement etc.

martinsumner added a commit that referenced this pull request Sep 2, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants