basho / riak-erlang-client

The Riak client for Erlang.
Apache License 2.0
311 stars 188 forks source link

RFC - DO NOT MERGE - Test deletion of object version (i.e. vclock) corresponding to CRDT version (i.e. context) #317

Closed lucafavatella closed 7 years ago

lucafavatella commented 8 years ago

The riakc_pb_socket:fetch_type/{3,4} function appears not to return the vclock of the Riak KV object hence the user cannot (naively) delete the object at the vclock corresponding to a specific CRDT context being therefore induced to delete the object using the unsafe riakc_pb_socket:delete/3 rather than riakc_pb_socket:delete_vclock/4 - safer as catering for concurrent writes. In order to delete the Riak KV object corresponding to a certain CRDT context the user has the only option of retrieving the server-side representation of the object via mapreduce, compare CRDT contexts (between the one to-be-deleted and the one retrieved server-side) and - if equal - call riakc_pb_socket:delete_vclock/4 passing the retrieved server-side vclock.

This PR adds tests for the above.

I expect this PR not to be merged, as in my opinion it highlights that there is room for improvement in the user API. I would expect either:

I understand both options require changes in the Protocol Buffers messages definition.

I partially understand the complexity in mapping from CRDT context to Riak KV object vclock because in a corner case the Riak object could contain not simply a CRDT e.g. set but unexpected siblings e.g. a set CRDT, a map CRDT, a register CRDT, a non-CRDT (I read of this corner case in the Riak KV server code). But I guess it could be handled returning an error probably as if if happen the user has probably got more serious issues...


lucafavatella commented 8 years ago

Sample output of relevant tests (not from the Travis CI run of this PR):

    riakc_pb_socket_tests:1369: integration_tests (delete with vclock set with context)...test/riakc_pb_socket_tests.erl:1382:<0.450.0>: The set with context <<131,108,0,0,0,1,104,2,109,0,0,0,8,35,9,254,249,102,157,123,137,97,1,106>> is empty - attempt once to delete the corresponding object. In order to do so, on the server: fetch the latest object by key (mapreduce), assert it has no siblings, assert it stores a CRDT, extract object vclock and CRDT context, finally return to the client. If successfully returned object vclock and CRDT context, on the client: if the CRDT context is the expected one, delete the object specifying the vclock detected as corresponding to the CRDT context.

[0.024 s] ok
    riakc_pb_socket_tests:1395: integration_tests (delete with vclock set with context - case determination of vclock corresponding to context failed for concurrent write)...[0.026 s] ok
    riakc_pb_socket_tests:1430: integration_tests (delete with vclock set with context - case delete failed for concurrent write)...[0.032 s] ok

(I had to comment get preflist test and add redundant and multiple items to hll(set)).

lucafavatella commented 8 years ago

I see that riakc_pb_socket:{modify,update}_type accept options, and I guess one option is return_body, but such option returns the CRDT - not the object. This is also evident in the Protocol Buffers API - ref1 ref2.

So mapreduce looks like the only option currently still.

lucafavatella commented 7 years ago

(It looks like the part that sets up Riak in this PR will soon be obsoleted by #319 - that is good.)

lucafavatella commented 7 years ago

I am currently rebasing this PR on current development code and I plan to open new PR.

lucafavatella commented 7 years ago

Closing and opening new PR.