Transaction commands support #86

Merged
merged 6 commits into from Jun 4, 2014

Conversation

Projects
None yet
3 participants
@larrylv
Contributor

larrylv commented Dec 26, 2013

The old way of #multi and #exec is not the exact
redis transaction commands. It just run every command after multi,
and save the result to buffer. Then when #exec is called, return
the buffer. So #discard couldn't work. (Actually it is not implemented)

The real #multi and #exec should be like this: save all the commands
after #multi, when #exec is called, run the commands saved. And if
#discard is called before #exec, client should forget all commands
after #multi, don't run any one of them.

My implementation is:

  • when #multi is called, alias all command methods to real_#{command}
  • and re-define the original command method: put the called command in a queue
  • when #exec is called, fetch every command from the queue, and run them.

Some specs codes are referenced from mock-redis.

@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius Jan 11, 2014

Collaborator

Hey, this is most certainly an interesting bug & fix. Thanks for putting this together! I certainly like the fact we're queueing up the commands to run rather than running them and then not being able to discard changes. (Also hadn't noticed #discard wasn't implemented. Yay for more compatibility!)

However, I'm not sold on the implementation looking at the diff. It's a novel approach creating method alias' on the fly, but I'm not convinced it's the right thing to be doing here. Especially as we then "revert" the aliases to get out of a multi command (be it via discard or exec). Feels wrong to be using the runtime in this way, if it was a one-off alias of everything it might be acceptable but I think there's other ways to solve this within the ruby toolkit before reaching runtime swizzling.

Almost feels more like something akin to middleware would be a better approach here to me. So it can decide whether to queue the command, or call it immediately and the command definitions are still unaware of whether they're in a multi block or not. (I agree with your implementation in that it doesn't require changes to all the existing command methods!)

So more along the existing #multi implementation, where we catch commands in #write but whilst we're in a multi block we then queue them. Seems like it would be a nicer way to implement this to me.

I just tried spiking this up, albeit without running it against your amended specs for multi/discard, and it feels nicer than alias' swizzling to me - https://github.com/guilleiguaran/fakeredis/compare/spike;multi-queue-commands. What do you think? Worth pursuing?

Thoughts @larrylv? @guilleiguaran too for that matter?

Collaborator

caius commented Jan 11, 2014

Hey, this is most certainly an interesting bug & fix. Thanks for putting this together! I certainly like the fact we're queueing up the commands to run rather than running them and then not being able to discard changes. (Also hadn't noticed #discard wasn't implemented. Yay for more compatibility!)

However, I'm not sold on the implementation looking at the diff. It's a novel approach creating method alias' on the fly, but I'm not convinced it's the right thing to be doing here. Especially as we then "revert" the aliases to get out of a multi command (be it via discard or exec). Feels wrong to be using the runtime in this way, if it was a one-off alias of everything it might be acceptable but I think there's other ways to solve this within the ruby toolkit before reaching runtime swizzling.

Almost feels more like something akin to middleware would be a better approach here to me. So it can decide whether to queue the command, or call it immediately and the command definitions are still unaware of whether they're in a multi block or not. (I agree with your implementation in that it doesn't require changes to all the existing command methods!)

So more along the existing #multi implementation, where we catch commands in #write but whilst we're in a multi block we then queue them. Seems like it would be a nicer way to implement this to me.

I just tried spiking this up, albeit without running it against your amended specs for multi/discard, and it feels nicer than alias' swizzling to me - https://github.com/guilleiguaran/fakeredis/compare/spike;multi-queue-commands. What do you think? Worth pursuing?

Thoughts @larrylv? @guilleiguaran too for that matter?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Feb 6, 2014

Owner

@caius 👍, feel free to take a decision on this

Owner

guilleiguaran commented Feb 6, 2014

@caius 👍, feel free to take a decision on this

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv May 22, 2014

Contributor

@caius I took your advice, and removed the aliases. I just re-write the write method, and queue command when we are in multi state.

No idea why the travis builds are not triggered. I run the tests locally, all green. :-)

Contributor

larrylv commented May 22, 2014

@caius I took your advice, and removed the aliases. I just re-write the write method, and queue command when we are in multi state.

No idea why the travis builds are not triggered. I run the tests locally, all green. :-)

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv May 27, 2014

Contributor

ping @caius :-)

Contributor

larrylv commented May 27, 2014

ping @caius :-)

@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius May 27, 2014

Collaborator

Hey,

I'm sailing through the scottish highlands with limited internet access and
no laptop this week, but I'll take a look when I'm back at a machine next
week if no-one else has beaten me to it :-)

C

Caius Durling
Transmitted from my handheld supercomputer
caius@caius.name
+44 (0) 7960 268 100
http://caius.name/

On 27 May 2014, at 08:58, Larry Lv notifications@github.com wrote:

ping @caius https://github.com/caius :-)


Reply to this email directly or view it on
GitHubhttps://github.com/guilleiguaran/fakeredis/pull/86#issuecomment-44245429
.

Collaborator

caius commented May 27, 2014

Hey,

I'm sailing through the scottish highlands with limited internet access and
no laptop this week, but I'll take a look when I'm back at a machine next
week if no-one else has beaten me to it :-)

C

Caius Durling
Transmitted from my handheld supercomputer
caius@caius.name
+44 (0) 7960 268 100
http://caius.name/

On 27 May 2014, at 08:58, Larry Lv notifications@github.com wrote:

ping @caius https://github.com/caius :-)


Reply to this email directly or view it on
GitHubhttps://github.com/guilleiguaran/fakeredis/pull/86#issuecomment-44245429
.

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv May 27, 2014

Contributor

@caius 😄 Have fun!

Contributor

larrylv commented May 27, 2014

@caius 😄 Have fun!

@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius Jun 4, 2014

Collaborator

@larrylv this is looking good indeed, although we've not got merge conflicts on master 😢 Can we get master merged in here and then I'll be happy to merge this down! Thanks 😀

Collaborator

caius commented Jun 4, 2014

@larrylv this is looking good indeed, although we've not got merge conflicts on master 😢 Can we get master merged in here and then I'll be happy to merge this down! Thanks 😀

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv Jun 4, 2014

Contributor

@caius Rebased my PR.

Contributor

larrylv commented Jun 4, 2014

@caius Rebased my PR.

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv Jun 4, 2014

Contributor

Seems like fakeredis is failing with rspec 3.0. Should we lock the rspec version on master?
https://travis-ci.org/guilleiguaran/fakeredis/builds

Contributor

larrylv commented Jun 4, 2014

Seems like fakeredis is failing with rspec 3.0. Should we lock the rspec version on master?
https://travis-ci.org/guilleiguaran/fakeredis/builds

@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius Jun 4, 2014

Collaborator

Just noticed that myself. Pinning it might be an idea for now.

Collaborator

caius commented Jun 4, 2014

Just noticed that myself. Pinning it might be an idea for now.

@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv Jun 4, 2014

Contributor

Cool. let me know when you have done that. I will rebase my PR, and let travis ci build again.

Contributor

larrylv commented Jun 4, 2014

Cool. let me know when you have done that. I will rebase my PR, and let travis ci build again.

@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius Jun 4, 2014

Collaborator

@larrylv Just pushed pinning to master. I'll try and sort out rspec 3 tonight, but that shouldn't hold up this PR!

Collaborator

caius commented Jun 4, 2014

@larrylv Just pushed pinning to master. I'll try and sort out rspec 3 tonight, but that shouldn't hold up this PR!

larrylv added some commits Dec 26, 2013

Add support for `#multi`, `#exec`, `#discard` commands.
The old way of `#multi` and `#exec` is not the exact
redis transaction commands. It just run every command after `multi`,
and save the result to buffer. Then when `#exec` is called, return
the buffer. So `#discard` couldn't work. (Actually it is not implemented)

The real `#multi` and `#exec` should be like this: save all the commands
after `#multi`, when `#exec` is called, run the commands saved. And if
`#discard` is called before `#exec`, client should forget all commands
after `#multi`, don't run any one of them.
@caius

This comment has been minimized.

Show comment
Hide comment
@caius

caius Jun 4, 2014

Collaborator

It might fail on rbx-2 if master is anything to go by, not going to block this for that though.

Many thanks for taking the time to implement & shepherd this in! 😀

Collaborator

caius commented Jun 4, 2014

It might fail on rbx-2 if master is anything to go by, not going to block this for that though.

Many thanks for taking the time to implement & shepherd this in! 😀

caius added a commit that referenced this pull request Jun 4, 2014

@caius caius merged commit 3ff3976 into guilleiguaran:master Jun 4, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@larrylv

This comment has been minimized.

Show comment
Hide comment
@larrylv

larrylv Jun 4, 2014

Contributor

:-) In case you didn't have time to implement some features, you could alwasy ping me. I will open a PR to let you review.

Contributor

larrylv commented Jun 4, 2014

:-) In case you didn't have time to implement some features, you could alwasy ping me. I will open a PR to let you review.

@larrylv larrylv deleted the larrylv:transaction-commands-support branch Jun 4, 2014

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