Skip to content
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.

Sign up for GitHub

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 is_crdt convenience fn #1081

Merged
merged 2 commits into from
Feb 19, 2015
+70 −2
Merged

add is_crdt convenience fn #1081

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7cae9e
add is_crdt convenience fn
zeeshanlakhani Feb 15, 2015
9f17bcc
updates based on review, add unit tests
zeeshanlakhani Feb 15, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 70 additions & 2 deletions src/riak_kv_crdt.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
-export([log_merge_errors/4, meta/2, merge_value/2]).
%% MR helper funs
-export([value/1, counter_value/1, set_value/1, map_value/1]).
%% Other helper funs
-export([is_crdt/1]).

-include("riak_kv_wm_raw.hrl").
-include("riak_object.hrl").
Expand Down Expand Up @@ -132,6 +134,21 @@ map_value(RObj) ->
{{_Ctx, Map}, _Stats} = value(RObj, ?MAP_TYPE),
Map.

%% @doc convenience function for (e.g.) Yokozuna. Checks the bucket props for
%% the object, if it has a supported datatype entry, returns true; otherwise
%% false if not a 2.0 CRDT.
-spec is_crdt(riak_object:riak_object()) -> boolean()|{error,_}.
is_crdt(RObj) ->
Bucket = riak_object:bucket(RObj),
case riak_core_bucket:get_bucket(Bucket) of
Copy link
Member

Choose a reason for hiding this comment

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

Why the case statement and not just:

BProps = riak_core_bucket:get_bucket(Bucket),

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda stole most of this from https://github.com/basho/riak_kv/blob/feature/zl/helper-fn-check-if-object-bucket-props-supports-crdt/src/riak_kv_crdt.erl#L103 haha, which probably has it just for the guard. But, you're right, the case is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at other spots, e.g.

riak_kv/src/riak_kv_util.erl

Line 163 in ec7e525

-spec consistent_object(binary() | {binary(),binary()}) -> true | false | {error,_}.
, I decided to keep the case, but actually have an error case if BProps is not a list for some reason.

BProps when is_list(BProps) ->
Type = proplists:get_value(datatype, BProps),
Mod = riak_kv_crdt:to_mod(Type),
supported(Mod);
{error, _}=Err ->
Err
end.

%% @TODO in riak_dt change value to query allow query to take an
%% argument, (so as to query subfields of map, or set membership etc)
-spec crdt_value(module(), error | {ok, {dict(), crdt()}}) ->
Expand Down Expand Up @@ -492,6 +509,57 @@ get_context(Type, Value) ->
%% ===================================================================
-ifdef(TEST).

is_crdt_test_() ->
{setup,
fun() ->
meck:new(riak_core_bucket),
meck:new(riak_core_capability, []),
meck:expect(riak_core_capability, get,
fun({riak_kv, crdt}, []) ->
[pncounter,riak_dt_pncounter,riak_dt_orswot,
riak_dt_map];
(X, Y) -> meck:passthrough([X, Y]) end),
ok
end,
fun(_) ->
meck:unload(riak_core_capability),
meck:unload(riak_core_bucket)
end,
[
?_test(begin
meck:expect(riak_core_bucket, get_bucket,
fun(_Bucket) -> [{datatype, foo}] end),
Bucket = {<<"counterz">>, <<"crdt">>},
BTProps = riak_core_bucket:get_bucket(Bucket),
?assertEqual(foo, proplists:get_value(datatype, BTProps)),
?assertNot(is_crdt(riak_object:new(Bucket, <<"k1">>, hello)))
end),
?_test(begin
Bucket = {<<"t">>, <<"bucketjumpy">>},
?assertNot(is_crdt(riak_object:new(Bucket, <<"k1">>, hi)))
end),
?_test(begin
meck:expect(riak_core_bucket, get_bucket,
fun({<<"maps">>, _Name}) -> [{datatype, map}];
({<<"sets">>, _Name}) -> [{datatype, set}];
({<<"counters">>, _Name}) -> [{datatype, counter}];
({X, Y}) -> meck:passthrough([X, Y]) end),
Bucket1 = {<<"maps">>, <<"crdt">>},
Bucket2 = {<<"sets">>, <<"crdt">>},
Bucket3 = {<<"counters">>, <<"crdt">>},
BTPropsMap = riak_core_bucket:get_bucket(Bucket1),
BTPropsSet = riak_core_bucket:get_bucket(Bucket2),
BTPropsCounter = riak_core_bucket:get_bucket(Bucket3),
?assertEqual(map, proplists:get_value(datatype, BTPropsMap)),
?assertEqual(set, proplists:get_value(datatype, BTPropsSet)),
?assertEqual(counter,
proplists:get_value(datatype, BTPropsCounter)),
[?assert(is_crdt(riak_object:new(B, K, V)))
|| {B, K, V} <- [{Bucket1, <<"k1">>, hi},
{Bucket2, <<"k2">>, hey},
{Bucket3, <<"k3">>, hey}]]

end)]}.

-ifdef(EQC).
-define(QC_OUT(P),
Expand All @@ -507,13 +575,13 @@ eqc_test_() ->
?_test(?TIMED_QC(prop_binary_roundtrip()))}.

prop_binary_roundtrip() ->
?FORALL({_Type, Mod}, oneof(?MOD_MAP),
?FORALL({_Type, Mod}, oneof(?MOD_MAP),
Copy link
Member

Choose a reason for hiding this comment

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

Is your emacs set to automatically remove trailing whitspace on save?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I know it can cause confusion. Should I turn it off?

Copy link
Member

Choose a reason for hiding this comment

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

I turned mine off after working on some old files and making every line into the diff, despite a small change. I guess I'm indifferent about it in general (I argued that having the autosave feature was a good thing since trailing whitespace is "bad"), though it can make review harder for more complex changes since it draws the reviewers attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... for larger diffs w/ older files, I'll make sure to turn it off. I also believe trailing whitespace is "bad."

begin
{ok, ?CRDT{mod=SMod, value=SValue}} = from_binary(to_binary(?CRDT{mod=Mod, value=Mod:new()})),
conjunction([{module, equals(Mod, SMod)},
{value, Mod:equal(SValue, Mod:new())}])
end).


-endif.
-endif.