Merge Strategy Behavior

[tburghart]

Discussion of the MergeStrategyBehaviour.md document.

[tburghart]

I think this is definitely the way to go, though I'd normalize and generalize the callbacks.

One possible approach follows, with some pattern changes.

Result Patterns

I've changed all functions to return one of:

• {ok, Result :: term()}
  • Sucess with a single result object.
• atom()
  • Result of a funtion that has no explicit error condition.
• {error, Reason :: term()}
  • Standard Erlang error result.

Context

I've generalized the #crdt_op{} type to a generic context() that only needs to be recognized by the callback module, allowing for as-yet-unknown use cases as well as more flexible CRDT contexts.

Additionally, I've defined the result of handle_put_request_body/2 to be either a riak_object:riak_object() or a tagged tupple bundling an intermediate object (most likely a riak_object:riak_object(), but possibly some richer type) with a context(), the latter to be finalized by process_update/2.

Question:
Is there a case where we need an additional merge_content/3 operation that adds the current context (if returned from handle_put_request_body/2) to the parameters of merge_content/2, or should the handling module always be able to ascertain what it needs to know from the content objects?

Module Flexibility

I've added the object being operated upon to the assign_dot/1 and reconcile_strategy/1 accessors, allowing for the implementing module to support multiple types.

If a module only ever returns one (constant) result from these operations, it can simply ignore the parameter.

%% @doc Riak KV Merge Strategy Behavior.
%%
%% === Result patterns ===
%%
%%  {ok, Result :: term()}
%%  atom()
%%  {error, Reason :: term()}
%%
%% === Extra Information ===
%%
%% The {@link context()} type is used to convey contextual information across
%% callbacks in a form understood by the callback module implementation.
%% The calling process passes this information uninterpretted.
%%
%% The {@link context_obj()} tagged tupple provides an unambiguous pattern
%% for matching a {@link intermediary()} (likely a {@link riak_obj()}) bundled
%% with an opaque {@link context()}.
%%
%% The callback module's `process_update/2', `assign_dot/1', and
%% `reconcile_strategy/1' functions MUST all handle ANY {@link subject_obj()}
%% returned from `handle_put_request_body/2' appropriately.
%%
-module(riak_kv_merge_strategy).

-export_type([
    context/0,
    context_obj/0,
    merge_opt/0,
    strategy/0
]).

-type context() :: term().      % Effectively opaque.
-type merge_opt() :: term().    % Allow for structured data.
-type strategy() :: most_recent | merge.

-type simple_obj() :: riak_obj().
-type context_obj() :: {ctx, intermediary(), context()}.
-type intermediary() :: term(). % Likely riak_obj(), but not explicit
-type subject_obj() :: simple_obj() | context_obj().

-type obj_content() :: riak_object:r_content().
-type obj_value() :: riak_object:value().
-type riak_obj() :: riak_object:riak_object().
-type vnode_id() :: riak_kv_vnode:vnode_id().

-callback handle_get_response_body(
    Object :: riak_obj(), MergeOption :: merge_opt() ) ->
        {ok, Result :: riak_obj()} | {error, Reason :: term()}.

-callback handle_put_request_body(
    Object :: riak_obj(), Value :: obj_value() ) ->
        {ok, Result :: subject_obj()} | {error, Reason :: term()}.

-callback merge_content(
    ObjContL :: obj_content(), ObjContR :: obj_content() ) ->
        {ok, Result :: obj_content()} | {error, Reason :: term()}.

-callback process_update(
    Object :: subject_obj(), VNodeID :: vnode_id() ) ->
        {ok, Result :: riak_obj()} | {error, Reason :: term()}.

-callback assign_dot(Object :: subject_obj() ) -> boolean().

-callback reconcile_strategy(Object :: subject_obj() ) -> strategy().

[martinsumner]

I'm happy with your changes. There will need to be a caveat that any module introduced in Riak 4.0 will need to output a context() that is a crdt_op record - as 4.0 will need to work in a mixed cluster with 3.4 nodes, and a 3.4 vnode would crash on receipt of a context other than a crdt_op.

I don't think there should be a merge_content/3. At the moment merge_content can get called from multiple places. For a vnode_put the context exists on all vnodes (but is only used by the coordinating vnode to apply the update after the merge, and is ignored by other vnodes). The context isn't present when handling a replicated PUT, or a read repair PUT, or when called from the GET_FSM etc. I think it is cleaner to reason about, if the context() is only used by a coordinating vnode in process_update, and merge_content never sees context.

[aramallo]

I really like this approach @martinsumner ! 👏🏼

[aramallo]

it would be ideal to pair this at the client side with a corresponding behaviour, so extending Riak with types would imply developing an implementation of riak_kv_merge_strategy and the client-side behaviour.