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

Add memcache increment and decrement #1135

Closed
wants to merge 30 commits into from
Closed

Conversation

iwalz
Copy link
Contributor

@iwalz iwalz commented Aug 22, 2013

This is a fix for #1083. It looks like the suggested changes for memcached are already done in 2.0 (if you want to I can prepare a PR against that too). I'm sorry about the formatting issue for the PR review, tried to fix it but seem to be screwed up anyways ... I was not sure where to send the PR against and how to handle phalcon.c in a PR. Since incr and decr are atomic, this is very useful i guess.

I thought about error handling in that implementation, but increase or decrease a non-numeric value simply raises a notice (which I was not able to prevent without shutup in userspace) and a not-existent value returns false. I can't really think of any other error-related use case.

@phalcon
Copy link
Collaborator

phalcon commented Aug 23, 2013

Since those methods are only implemented in the memcache adapter, it would make the other adapters look a bit different

@iwalz
Copy link
Contributor Author

iwalz commented Aug 23, 2013

I thought about an implementation for the other adapters too, but there's a significant difference between the memcache service and the others. Following example:

$cache->set('foo', $cache->get('foo') + 1);

Can be done in any adapter this way. But memcache is a bit special, increment and decrement are the only atomic operations in memcache. Other adapters are a kind of atomic by nature, but memcache isn't - so these operations are very usefull, especially for memcache users. APC didn't even provide an increment/decrement function itself. For XCache it's the same as for memcache, so it makes sense there. You want me to add this functionality for XCache the same way and for the others "wrappers" for the example above?

What about the 2.0 Branch? Do you want a port for that?

@phalcon
Copy link
Collaborator

phalcon commented Aug 23, 2013

Yes, I think those methods need to be implemented in every adapter even if the result functionality is not atomic, that's is something we could mention in the docs and the developer can take decisions on it.

Regarding 2.0, this version will be written in Zephir, maybe this is not the moment to start a port since the language is still in development. Thanks

@iwalz
Copy link
Contributor Author

iwalz commented Aug 23, 2013

Okay, let me add xcache later today and the other through the weekend. Are you in general fine with the implementation itself?

@iwalz
Copy link
Contributor Author

iwalz commented Aug 24, 2013

Dev is still ongoing, I'll ping you once it's ready for review again. Thanks for the hints.
I got really strange behaviours on xcache - different behaviours on different travis boxes and even others on my local dev environment. It's conflicting with APC and some versions seem to not have an inc and dec function - but the doc is quite ... Let's say there is not much for xcache ;-) So I needed to push often to trigger a travis build and fix errors that are happening there and not locally.

@phalcon
Copy link
Collaborator

phalcon commented Sep 4, 2013

Did you finish your development? Can be this merged?

@iwalz
Copy link
Contributor Author

iwalz commented Sep 4, 2013

Not yet, file and mongo still needs to be done. I've moved to a new
appartment and there's no internet connection till 9.9 - will finish it as
soon as possible.

@phalcon
Copy link
Collaborator

phalcon commented Sep 6, 2013

Thanks, please let us know when it's ready

@phalcon
Copy link
Collaborator

phalcon commented Sep 6, 2013

Also, could you submit this to the 1.3.0 branch?

@iwalz
Copy link
Contributor Author

iwalz commented Sep 16, 2013

@phalcon I'm struggling a bit with the mongo implementation. I got weird travis behaviours and the mongo db tests where prefixed with a "_" before (to not execute the tests). The test suite runs fine locally but from "Cannot convert to ordinal value" to segmentation faults, everything is possible on travis ... Can someone have a quick look on the mongodb increment & decrement and point me to the right direction?

@iwalz
Copy link
Contributor Author

iwalz commented Sep 27, 2013

ping @phalcon
Should be fine for review again. The travis failures seem not related.
I had a very strange looking commit once I sent the PR to 1.3 - will investigate

@phalcon
Copy link
Collaborator

phalcon commented Oct 2, 2013

I'm going to close this, could you please submit this in 1.3.0?

@iwalz
Copy link
Contributor Author

iwalz commented Oct 2, 2013

Yes, you can close this. I'm still investigating how to solve my conflicts the best, otherwise I've to redo it for 1.3.0. Any suggestion how to fix it the best? Mergeing seem not possible at all ...

# git clone https://github.com/phalcon/cphalcon.git .
# git checkout 1.2.3
# git checkout 1.3.0
# git merge 1.2.3

Gives me a conflict for ~50 files

@ghost
Copy link

ghost commented Oct 3, 2013

You can try to create the patch for 1.2.3 and your branch, then checkout to 1.3.0 and try to apply the patch, This will result in conflicts, but there should be much less conflicts than you have now.

@iwalz iwalz closed this Oct 13, 2013
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.

2 participants