chitika / cberl

NIF based Erlang bindings for Couchbase
Other
72 stars 65 forks source link

After updated libcouchbase cberl:append not work #59

Closed cclam0827 closed 9 years ago

cclam0827 commented 9 years ago

After update libcouchbase from 2.4.1 to 2.4.4, I found that append function always return unknow error. Is somebody also have this issue?

In CentOS : cberl:append(bucket,0, <<"test">>, <<"test">>). {error,{unknown_error,58}}

In OSX: cberl:append(bucket,0, <<"test">>, <<"test">>). {error,unknown_error}

wcummings commented 9 years ago

58 is the detailed error code

  /**An option was passed to a command which is incompatible with other
     * options. This may happen if two fields are mutually exclusive */ \
    X(LCB_OPTIONS_CONFLICT, 0x3A, LCB_ERRTYPE_INPUT, \
      "The operation structure contains conflicting options")

I'll look into this

wcummings commented 9 years ago

Looks like the newer version is stricter, flags should not be set for append/prepend:

    case LCB_APPEND:
    case LCB_PREPEND:
        if (cmd->exptime || cmd->flags) {
            return LCB_OPTIONS_CONFLICT;
        }
        break;
cclam0827 commented 9 years ago

Can we keep the api append/4? like this

append(PoolPid, Cas, Key, Value) ->
   store(PoolPid, append, Key, Value, none, 0, 0).

or

append(PoolPid, Cas, Key, Value) ->
    append(PoolPid, Key, Value).
append(PoolPid, Key, Value) ->
    store(PoolPid, append, Key, Value, none, 0, 0).
sjmackenzie commented 9 years ago

Typically is bad form to breaking public APIs. Unless of course there is an overriding consensus.

wcummings commented 9 years ago

It's easy enough to avoid in this case, but it will happen from time to time. I will add deprecated functions for append and prepend which accept an unused CAS value

sjmackenzie commented 9 years ago

Understood, rock and a hard place, never easy, though this project is starting to become more mature with production code using it as a dependency so public API stability is rather important indeed.

I must thank you for your amazing contribution, most appreciated :)

hlieberman commented 9 years ago

@sjmackenzie, it's my fault. I normally try to keep us pretty close to SemVer, and I didn't look closely at the patch set to see that there was a breaking change in there. We're not always going to be able to avoid them, but I'll try to make sure that they're not a surprise, at least.

vincentdephily commented 9 years ago

With that and returning {ok,CAS} and having a flexible options argument for storage functions, it would seem like a good idea to bundle as many pending API changes as desired into a 1.1 branch, and keep non-api-breaking patches for master in the meantime ? You can then decide how long the 1.0 branch should be supported.

sjmackenzie commented 9 years ago

my personal preference is to use the zeromq development 'contract' inthat there is only the master branch on the canonical repository and stable APIs dont change.APIs have a lifecycle, being annotated with 'experimental', 'stable', 'depricated' and 'legacy'. Having a predictable stable API is a softwares greatest assert. Im terribly sorry that you're building on couchbase' quicksand foundation and i'll reuse the expression, are caught between a rock and a hard place. Have a read of the ZeroMQ development methodology: http://rfc.zeromq.org/spec:16

sjmackenzie commented 9 years ago

(forgive my mobile phone typos)

wcummings commented 9 years ago

I've long considered creating a "v2" API, while maintaining this one. Now might be the right time to do that, with the current API being 'stable' and the new API being 'experimental'. The biggest issue is that the current API doesn't provide for many ways to extend it w/o breaking compatibility.

I have a changeset for the deprecated append/prepend functions, so I will merge those in shortly and we can hold off and put the CAS changes into a new experimental branch.

Is this agreeable? @sjmackenzie @vincentdephily @hlieberman

sjmackenzie commented 9 years ago

Yes its a good way albeit using a different name. Names, if used in the context of C4, stay around forever :)