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

Adds Lua transaction support #2701

Closed

Conversation

@josiahcarlson
Copy link

josiahcarlson commented Jul 29, 2015

Adds 2 commands: EVALTXN and EVALSHATXN - runs Lua scripts in a transaction
Adds 1 script sub-comand: SCRIPT ROLLBACK - kills scripts that have written if they are execued as a transaction
Adds 1 configuration option: lua-all-transactions - for specifying that all EVAL/EVALSHA should be transactions too

@nicpottier
Copy link

nicpottier commented Jul 29, 2015

+1 to the inclusion of this feature (though can't comment on this particular implementation)

@flip111
Copy link

flip111 commented Jul 30, 2015

👍

@@ -4440,6 +4440,110 @@ void restoreCommand(client *c) {
server.dirty++;
}

void prepareRollback(struct redisCommand *cmd, client *c) {

This comment has been minimized.

Copy link
@badboy

badboy Jul 30, 2015

Contributor

Any reason for this being in cluster.c? It does work in normal Redis, right?

This comment has been minimized.

Copy link
@josiahcarlson

josiahcarlson Jul 30, 2015

Author

Three reasons:

  1. I didn't know where else to put it (so I am happy to move it)
  2. The current implementation is an automatic dump/restore
  3. Most of the guts of dump/restore are defined in cluster.c, implemented earlier in the file

Where should I move it?

This comment has been minimized.

Copy link
@badboy

badboy Jul 30, 2015

Contributor

Not sure where to move. Just wondered if this was a thoughtful decision or just happened to be there for no reason.

@josiahcarlson
Copy link
Author

josiahcarlson commented Jul 30, 2015

The implementation needs to be better long-term. Someone else will have to judge whether the implementation with documentation/caveats is good enough for now.

josiahcarlson referenced this pull request in josiahcarlson/redis Aug 8, 2015
Features:
* New commands: EVALTXN, EVALSHATXN, ROLLBACK
* EVALTXN and EVALSHATXN allow writes to be rolled back via ROLLBACK command
  outside of scripts, and redis.ROLLBACK() inside Lua scripts

Todo:
* Valgrind
* More tests
* Use hash/dict instead of linked list for rollback items
* Maybe merge ROLLBACK command and redis.ROLLBACK(), or hook in some other place
* Other rollback methods instead of taking a dump of keys written to (fork,
  slave read, pre-persisted from AOF, ...)
@tzudot
Copy link

tzudot commented Aug 12, 2015

👍

@pankajsinghal
Copy link

pankajsinghal commented Nov 13, 2017

Bumping this thread again. There isn't any update here for a long time.

Is this feature still a priority?

@josiahcarlson
Copy link
Author

josiahcarlson commented Nov 13, 2017

Is this feature still a priority?

For me, yes. Anyone else involved with this project? Clearly not (for evidence, see the 2 years of no response).

@pankajsinghal
Copy link

pankajsinghal commented Nov 14, 2017

Transactions are clearly needed, there's no denying that.

Any suggestions on how can we get its priority increased?

@antirez
Copy link
Contributor

antirez commented Nov 14, 2017

Hello, as @josiahcarlson guessed (and basically knowns), the current direction of the project does not include adding rollbacks to scripting / transactions, but retaining instead the simpler transaction model that we implemented initially (and that are more and more replaced by Lua scripts these days, in most users code). Design questions like that are a fact of but objective and subjective evaluations, so these debate may be endless. Let's just say that for me personally this is not a good idea, and moreover there is strong evidence that the user base, for the most part, is not interested in such a feature. Moreover there is no obvious good implementation for the feature. Based on all these observations, I'm closing this PR. Thanks for your POV and contribution.

@antirez antirez closed this Nov 14, 2017
@josiahcarlson
Copy link
Author

josiahcarlson commented Nov 14, 2017

@antirez I wanted to give you a path where this was fully implementable without too many changes (the original patch, as above), with a path moving forward (subsequent conversations and emails).

I'm sorry you weren't interested in those conversations or emails.

@antirez
Copy link
Contributor

antirez commented Nov 14, 2017

Apart from the fact that I replied to the initial emails, what is the goal of trying to understand a complex path to an implementation which is, IMHO, semantically not useful. Again, opinions may vary, but given that I handle the project, I've to transfer my vision (after that I inform it with other arguments). And I believe that this feature is still, mostly, not good for Redis users and Redis use cases. And given that any implementation requires some serious tradeoff (for the nature of the feature itself), the exact implementation was a successive path that I did not explored because stopped at the premises already.

@antirez
Copy link
Contributor

antirez commented Nov 14, 2017

Worth to note how also if I would follow every lead about potentially useful things, regardless of what I think and sense in the community, Redis would be at this point total bloatware after 8 years. Even resisting to most things, we are at 80k lines of code, so the project has something like 2x the lines of code it had when this feature was proposed.

@josiahcarlson
Copy link
Author

josiahcarlson commented Nov 14, 2017

I'm really sorry you think, "IMHO, semantically not useful". My Lua scripts against real transactions were half as long. Even beyond the data integrity, writing half as much Lua is nice.

As I said above, "I'm sorry you weren't interested in those conversations or emails." Because you weren't interested. And it is totally, 100% okay that you didn't and don't want real transactions in Redis, it is your project.

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.