Honor select disposition in transactions by slorello89 · Pull Request #2322 · StackExchange/StackExchange.Redis (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #2321
For transactions, if you remove SELECT from the command map, it will still try to send one if you issue an unknown/eval/evalsha command - this adds a bit of logic when enqueuing messages to interrogate the multiplexer to see if SELECT is in the command map.
| { |
|---|
| (_pending ??= new List<QueuedMessage>()).Add(queued); |
| switch (message.Command) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the test inside the switch? I'd rather not execute the IsAvailable check etc unless we think there's a good need; other than that: makes sense
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this change, but: unless I'm misreading the code, it looks like we'd issue this even on "cluster"; open question: should we also add a single-DB check? we usually know the DB count, either from server metadata (when available) or from the server type (note: DB count can be unknown)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good wrt to rearranging things.
WRT preventing an issue with cluster - what do you think of this new approach?
Dropping a reminder: Release notes please! Happy to add when ready at the end too, just making sure we remember :)
Dropping a reminder: Release notes please! Happy to add when ready at the end too, just making sure we remember :)
sorry @NickCraver - added :)
The more I look at this, the more I wonder whether this RedisTransaction code is actually needed, or the right place; the comparison is to here:
| case RedisCommand.EVALSHA: |
|---|
- IMO RediaTransaction might not need to do anything here, if we can refactor that linked code above a bit to handle sub-messages. I can look on Monday
The more I look at this, the more I wonder whether this
RedisTransactioncode is actually needed, or the right place; the comparison is to here:
case RedisCommand.EVALSHA:
- IMO
RediaTransactionmight not need to do anything here, if we can refactor that linked code above a bit to handle sub-messages. I can look on Monday
I see - it does seem a tad bit out of place. The code does intersect there to set the database to dirty I think - so it might just work if we ripped this bit out.
NickCraver added a commit that referenced this pull request
