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

Return CAS value #87

Closed
wants to merge 5 commits into from
Closed

Return CAS value #87

wants to merge 5 commits into from

Conversation

wcummings
Copy link
Contributor

@wcummings wcummings commented Dec 6, 2016

Tests pass, dialyzer is happy

This is a breaking change, but I agree w/ @vincentdephily that it is important enough. Because it affects so many functions, the alternatives would severely muddy the interface (for ex we could create a new set of store functions in a cberl_v2 module, or a new set of functions w/ a different arity, essential a "return a cas" flag). This should be accompanied by a version bump.

#86

cc @vasumahesh1

P.S. "" doesnt give you the default bucket anymore, this includes a commit to pass "default" instead

vincentdephily and others added 4 commits November 3, 2016 15:38
This saves a roundtrip when you set/add an item that you'll want to modify later.

This is a backward-incompatible change, but it seems important enough and seems to be
a coding oversight as most of the work was already done (but thrown away) in store_callback().
@wcummings
Copy link
Contributor Author

I notice https://github.com/orgs/chitika/people is empty now, might be time to start using my repo as the trunk 😂

@vincentdephily
Copy link
Contributor

@wcummings in the original PR (#62) you suggested merging the optional arguments together into a map (or proplist or record or...), I think that would work well and we can do this fairly elegantly:

% @deprecated
store(PoolPid, Op, Key, Value, TranscoderOpts, Exp, Cas) ->
    execute(PoolPid, {store, Op, Key, Value, #{trancoder => TranscoderOpts, exp => Exp, cas => Cas}).
% New default function
store(PoolPid, Op, Key, Value, Opts) ->
    execute(PoolPid, {store, Op, Key, Value, Opts}).
% Simple version
store(PoolPid, Op, Key, Value) ->
    execute(PoolPid, {store, Op, Key, Value, #{}}).
% Some functions require some where trickery but shouldn't need a name change
append(PoolPid, Cas, Key, Value) when is_integer(Cas) ->
    store(PoolPid, append, Key, Value, {cas => Cas});
append(PoolPid, Key, Value, Opts) ->
    store(PoolPid, append, Key, Value, Opts).
append(PoolPid, Key, Value) ->
    store(PoolPid, append, Key, Value, #{}).

From there on, the API should be a bit more future-proof and it'll be easy to add a returncas boolean opt (checked by cberl_worker:store()) so users can opt into the returncas behavior.

@wcummings
Copy link
Contributor Author

wcummings commented Dec 21, 2016

@vincentdephily 👍 I can make this happen

@wcummings
Copy link
Contributor Author

wcummings commented Dec 28, 2016

@vincentdephily So i think this basically works, but the problem I've noticed is situations like this:

add(PoolPid, Key, Value) ->
    add(PoolPid, Key, Value, []).

add(PoolPid, Key, Value, Opts) when is_list(Opts) ->
    store(PoolPid, add, Key, Value, Opts);

%% @equiv add(PoolPid, Key, Exp, Value, standard)
%% @deprecated
add(PoolPid, Key, Exp, Value) ->
    store(PoolPid, add, Key, Value, [{exp, Exp}]).

If the key is a string (list of chars) the guard won't save us. Similarly, a value can be an integer, just like an expiration. I think the best solution is to simply remove the newly deprecated functions, but I'm open to suggestions. Since a value can be basically anything it's pretty hard to resolve this "clash". Even a tagged tuple could potentially be a valid value, depending on the transcoder being used.

The options I see are:

  • Break compatibility (what I have committed so far), bump the version accordingly.
  • Keep Exp as a required argument for replace, add & set. Confusing since its inconsistent w/ the other functions.
  • Rename some functions.

I'll push my changes shortly.

@wcummings
Copy link
Contributor Author

OK after some deliberation I'm going with the original change, it breaks compatibility but it seems that was unavoidable and this is just a lot simpler and probably an easier change for people. It's in my repo w/ a tagged release. https://github.com/wcummings/cberl

@wcummings wcummings closed this Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants