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.

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

Perf/zl/remove unnecessary delete to solr idx when crdt or sc #452

Conversation

zeeshanlakhani
Copy link
Contributor

Fixes https://bashoeng.atlassian.net/browse/RIAK-1504.

Improves performance by avoiding an extra delete operation to remove siblings through Solr on the put path when riak datatypes or strong consistency is used. Initial patch/issue was discovered by @drewkerrigan, https://github.com/basho-labs/yokozuna_perf_patch.

@Basho-JIRA
Copy link

#452

_[posted via JIRA by Zeeshan Lakhani]_

%% strong consistency.
-spec is_datatype_or_consistent(obj()) -> boolean().
is_datatype_or_consistent(Obj) ->
case riak_kv_crdt:value(Obj) of

Choose a reason for hiding this comment

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

Similar to the is_strongly_consistent/1 below, you could check the bucket properties only to determine if it's a CRDT. Calling riak_kv_crdt:value/1 will perform a deserialization of the value, which is extra work if you are not intending to use the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will do.

Zeeshan Lakhani
programmer |
software engineer at @basho |
org. member/founder of @papers_we_love |
twitter => @zeeshanlakhani

On Feb 14, 2015, at 3:50 PM, Sean Cribbs notifications@github.com wrote:

In src/yz_kv.erl:

@@ -460,3 +463,131 @@ is_owner_or_future_owner(P, Node, Ring) ->
-spec is_service_up(atom(), node()) -> boolean().
is_service_up(Service, Node) ->
lists:member(Service, riak_core_node_watcher:services(Node)).
+
+%% @Private
+%%
+%% @doc Check if object has 2.0 CRDT datatype entry or property for
+%% strong consistency.
+-spec is_datatype_or_consistent(obj()) -> boolean().
+is_datatype_or_consistent(Obj) ->

  • case riak_kv_crdt:value(Obj) of
    Similar to the is_strongly_consistent/1 below, you could check the bucket properties only to determine if it's a CRDT. Calling riak_kv_crdt:value/1 will perform a deserialization of the value, which is extra work if you are not intending to use the result.


Reply to this email directly or view it on GitHub.

@zeeshanlakhani
Copy link
Contributor Author

@borshop will now fail until basho/riak_kv#1081 is reviewed/merged, but eunit tests pass locally.

@zeeshanlakhani zeeshanlakhani force-pushed the perf/zl/remove-unnecessary-delete-to-solr-idx-when-crdt-or-sc branch from 8390aff to 13b819a Compare February 15, 2015 17:19
%% @doc Set yz_solr:index delete operation(s) on write_reason.
-spec delete_operation(obj(), put|handoff|anti_entropy, [doc()], bkey(), lp()) ->
[]|[{id, _}]|[{siblings, _}].
delete_operation(Obj, put, Docs, BKey, LP) ->

Choose a reason for hiding this comment

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

Can you explain why handoff or anti_entropy should result in cleanups? Shouldn't the same reasons apply in those cases as for put?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a conversation I had w/ @drewkerrigan, it was to keep the delete operation for handoff/anti_entropy as cleanup b/c there was an edge case where data-typed keys could actually have a siblings and "one of the test cases the client had was like deleting without using a context and then updating the datatype object."

Choose a reason for hiding this comment

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

Understood. I think we should fix the underlying problem in that case, but that should be ok for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, as there's another issue altogether.

@seancribbs
Copy link

Holding off final review on this for benchmarks.

@zeeshanlakhani
Copy link
Contributor Author

@seancribbs, yep, will post results here comparing develop to this branch.

@zeeshanlakhani
Copy link
Contributor Author

@seancribbs will reopen w/ a new PR... as the rebase w/ the changes to dev got a bit funky.

@zeeshanlakhani zeeshanlakhani deleted the perf/zl/remove-unnecessary-delete-to-solr-idx-when-crdt-or-sc branch February 21, 2015 23:08
@Basho-JIRA
Copy link

Updated fix version to 2.0.6 since the GH was included in release notes by Zeeshan.

_[posted via JIRA by Patricia Brewer]_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants