{"action":"created","issue":{"url":"https://api.github.com/repos/basho/riak_kv/issues/1404","repository_url":"https://api.github.com/repos/basho/riak_kv","labels_url":"https://api.github.com/repos/basho/riak_kv/issues/1404/labels{/name}","comments_url":"https://api.github.com/repos/basho/riak_kv/issues/1404/comments","events_url":"https://api.github.com/repos/basho/riak_kv/issues/1404/events","html_url":"https://github.com/basho/riak_kv/pull/1404","id":151102840,"number":1404,"title":"Bugfix/listkeys filters","user":{"login":"erikleitch","id":9666459,"avatar_url":"https://avatars.githubusercontent.com/u/9666459?v=3","gravatar_id":"","url":"https://api.github.com/users/erikleitch","html_url":"https://github.com/erikleitch","followers_url":"https://api.github.com/users/erikleitch/followers","following_url":"https://api.github.com/users/erikleitch/following{/other_user}","gists_url":"https://api.github.com/users/erikleitch/gists{/gist_id}","starred_url":"https://api.github.com/users/erikleitch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/erikleitch/subscriptions","organizations_url":"https://api.github.com/users/erikleitch/orgs","repos_url":"https://api.github.com/users/erikleitch/repos","events_url":"https://api.github.com/users/erikleitch/events{/privacy}","received_events_url":"https://api.github.com/users/erikleitch/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2016-04-26T10:53:40Z","updated_at":"2016-04-29T16:19:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/basho/riak_kv/pulls/1404","html_url":"https://github.com/basho/riak_kv/pull/1404","diff_url":"https://github.com/basho/riak_kv/pull/1404.diff","patch_url":"https://github.com/basho/riak_kv/pull/1404.patch"},"body":"**Context**\r\n\r\nstream_list_keys has always returned inconsistent results when used with TS data.  I've tracked this down to the coverage filter functions that are applied during the fold over keys on a vnode.  These filters check whether a key should be included, by checking if its hash falls within a specified hash range for a vnode.  These filters are applied to the storage keys in leveldb.  For TS, however, these filters need to be applied not to the storage key (which is the local key), but instead to the partition key, which can be derived from the local key by an appropriate transformation.\r\n\r\nThis PR addresses this inconsistency by changing the filter functions to use a key conversion function on the storage key prior to hashing. It is one of three PRs: riak_kv (https://github.com/basho/riak_kv/pull/1404), riak_core (https://github.com/basho/riak_core/pull/834) and riak_pipe (https://github.com/basho/riak_pipe/pull/105).  For reference, I include my diagram of the listkeys code path here:\r\n\r\n ![RiakTS Listkeys Path](https://gist.githubusercontent.com/erikleitch/8191db35b09bc8357df4/raw/69e7510c9233ecc3e35fd78a12807df6fab5514d/tslistkeys.png)\r\n\r\n**Changes in this repo**\r\n\r\n* **riak_kv_ts_svc.erl**<br>\r\n   * sub_tslistkeysreq now constructs a key conversion\r\nfunction (KeyConvFn) when invoked, that takes the decoded object key, sext\r\ndecodes it, and converts to a partition key, using riak_kv_ts_api:row_to_key with the\r\nMod and DDL specific to the table for which listkeys has been invoked.  This is passed as an argument to riak_client:stream_list_keys\r\n\r\n* **riak_kv_ts_api**<br>\r\n  * row_to_key/3 exported because it is needed in sub_tslistkeysreq when building the key conversion function\r\n\r\n* **riak_client**<br>\r\n * stream_list_keys/5 has been added, with KeyConvFn as 3rd arg, as \r\nthe function that all variants of stream_list_keys call down to.\r\nThis calls riak_kv_keys_fsm_sup:start_keys_fsm with KeyConvFn as an\r\nextra argument in the fsm startup arg list\r\n\r\n  * stream_list_keys/4, which in the riak_ee codebase was only called by sub_tslistkeysreq, has been modified to take KeyConvFn as its 3rd argument\r\n\r\n  * the interfaces to stream_list_keys/2 and stream_list_keys/3, which are used  elsewhere in\r\nthe riak_ee codebase, remain unchanged.  On invocation,\r\nthey construct a no-op key conversion function and pass it into stream_list_keys/4\r\n\r\n  * list_keys/4 has been modified to call start_keys_fsm with a no-op key conversion function, to ensure that list_keys also continues to work with this change\r\n\r\n* **riak_kv_keys_fsm**<br>\r\n  * init uses the KeyConvFn argument to construct a\r\ncoverage plan function that returns a proplist including the key\r\nconversion function, in addition to the usual vnode filter list.  This will\r\nbe needed later in riak_kv_vnode:handle_coverage_fold to construct the\r\nvnode filter function applied during the listkeys fold.\r\n\r\n* **riak_kv_index_fsm.erl, riak_kv_buckets_fsm.erl**<br>\r\n  * Because riak_core_coverage_fsm now expects a plan fn rather than a plan mod, init functions for these modules have also been modified to return a plan fun, so that we don't break range scans and bucket listing\r\n\r\n* **riak_kv_vnode.erl**<br>\r\n  * handle_coverage_fold has been modified to extract the key\r\nconversion function from the FilterVNodes proplist, and pass it into\r\n riak_kv_coverage_filter:build_filter, where it is used to construct an appropriate\r\nfilter\r\n\r\n* **riak_kv_coverage_filter.erl**<br>\r\n  * build_filter/4 has been added to use an explicit key conversion function\r\n\r\n  * build_filter/3 has been modified to pass a no-op key conversion function, to  preserve the functionality of other code that uses build_filter/3 directly\r\n\r\n  * build_preflist_fun and build_subpartition_fun now take KeyConvFn as an extra argument, and use it to convert the key before hashing\r\n\r\n**Notes**\r\n* I have verified in manual testing that streaming list keys now works consistently for TS buckets (ie, the changes here do what they are intended to do)\r\n* I have verified in manual testing that streaming list keys still works (and returns consistent results) for non-TS buckets (ie, the code changes here haven't broken listkeys for non-TS buckets, and incidentally that there isn't a separate problem with non-TS listkeys)\r\n* I have verified in riak_test that verify_listkeys now passes (note that there is also a bugfix/listkeys_filters branch of riak_ee that can be used in riak_test to verify the changes)\r\n* I still want to verify that stream_list_buckets works, but I haven't gotten to that yet\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/basho/riak_kv/issues/comments/215789868","html_url":"https://github.com/basho/riak_kv/pull/1404#issuecomment-215789868","issue_url":"https://api.github.com/repos/basho/riak_kv/issues/1404","id":215789868,"user":{"login":"hmmr","id":1107079,"avatar_url":"https://avatars.githubusercontent.com/u/1107079?v=3","gravatar_id":"","url":"https://api.github.com/users/hmmr","html_url":"https://github.com/hmmr","followers_url":"https://api.github.com/users/hmmr/followers","following_url":"https://api.github.com/users/hmmr/following{/other_user}","gists_url":"https://api.github.com/users/hmmr/gists{/gist_id}","starred_url":"https://api.github.com/users/hmmr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hmmr/subscriptions","organizations_url":"https://api.github.com/users/hmmr/orgs","repos_url":"https://api.github.com/users/hmmr/repos","events_url":"https://api.github.com/users/hmmr/events{/privacy}","received_events_url":"https://api.github.com/users/hmmr/received_events","type":"User","site_admin":false},"created_at":"2016-04-29T16:19:21Z","updated_at":"2016-04-29T16:19:21Z","body":"+1 7754045"}}