Initial support of transactions #5
Conversation
| } else { | ||
| __redisClusterSetError( | ||
| cc, REDIS_ERR_OTHER, | ||
| "No keys in command(must have keys for redis cluster mode)"); |
There was a problem hiding this comment.
These messages are not beautiful, but legacy... Maybe some other time, we can reformulate them.
hircluster.c
Outdated
| __redisClusterSetError(cc, REDIS_ERR_OTHER, | ||
| "only supports same slot transactions"); |
There was a problem hiding this comment.
Who only supports? Better use passive, e.g. "Only same slot transactions supported".
hircluster.c
Outdated
| if (cc->transaction_slot == TRANSACTION_STARTED) { | ||
| // Previous command was MULTI, use current slot as transaction slot | ||
| listNode *prev_command = listLast(cc->requests); | ||
| if (prev_command == NULL) { | ||
| __redisClusterSetError(cc, REDIS_ERR_OTHER, "transaction failure"); | ||
| goto error; |
There was a problem hiding this comment.
Can prev_command ever be NULL if we are in TRANSACTION_STARTED? (Maybe after reconnect?)
If it can't happen, maybe this should be an ASSERT instead?
There was a problem hiding this comment.
Should not be possible unless there is some fishyness. There are a lot of checks throughout the codebase and many where asserts could be more fitting, like if the clustercontext is null..which is everywhere.
I was thinking that when there are more tests to lean on we could make the internals more strict and assert where applicable
hircluster.c
Outdated
| // Send MULTI command to correct slot first | ||
| if (__redisClusterAppendCommand(cc, multi_command) != REDIS_OK) { | ||
| __redisClusterSetError(cc, REDIS_ERR_OTHER, | ||
| "transaction fail while sending MULTI"); |
tests/ct_transaction.c
Outdated
| status = redisClusterAppendCommand( | ||
| cc, "SET foo five"); // Not ok due to other slot | ||
| assert(status != REDIS_OK); | ||
| status = redisClusterAppendCommand(cc, "GET bar"); |
There was a problem hiding this comment.
I think it aligns better with the rest of the code like this:
status = redisClusterAppendCommand(cc, "SET foo five");
assert(status != REDIS_OK); // Not ok due to other slot
Handles MULTI, EXEC and DISCARD messages in the pipeline API using `redisClusterAppendCommand()` Only supports transactions to a single slot
|
@bjosv: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Handles MULTI, EXEC and DISCARD messages in the pipeline API
using
redisClusterAppendCommand()What the implementation does is that when a MULTI command is received it is not sent directly.
Instead its saved in a queue as normally done as well, but now also keeping its command string that will be sent later.
When the next command is received its slot is extracted, and used for the saved MULTI command as well.
The MULTI command is now sent to the correct node before the first command in the transaction.
When EXEC is sent this finishes the transaction state, and will receive the array of all responses.
Only supports transactions to a single slot
Only supports the pipeline API currently
Support for async and single sync api to be added