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
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@josiahcarlson

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

This comment has been minimized.

Show comment
Hide comment
@nicpottier

nicpottier Jul 29, 2015

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

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

@flip111

This comment has been minimized.

Show comment
Hide comment

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.

@badboy

badboy Jul 30, 2015

Contributor

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

@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.

@josiahcarlson

josiahcarlson Jul 30, 2015

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?

@josiahcarlson

josiahcarlson Jul 30, 2015

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.

@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.

@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

This comment has been minimized.

Show comment
Hide comment
@josiahcarlson

josiahcarlson 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.

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

Initial commit of Lua transactions
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

This comment has been minimized.

Show comment
Hide comment

tzudot commented Aug 12, 2015

👍

@pankajsinghal

This comment has been minimized.

Show comment
Hide comment
@pankajsinghal

pankajsinghal Nov 13, 2017

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

Is this feature still a priority?

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

Is this feature still a priority?

@josiahcarlson

This comment has been minimized.

Show comment
Hide comment
@josiahcarlson

josiahcarlson 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).

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

This comment has been minimized.

Show comment
Hide comment
@pankajsinghal

pankajsinghal Nov 14, 2017

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

Any suggestions on how can we get its priority increased?

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

Any suggestions on how can we get its priority increased?

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Nov 14, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@josiahcarlson

josiahcarlson 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.

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

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Nov 14, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Nov 14, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@josiahcarlson

josiahcarlson 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.

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