Honor select disposition in transactions by slorello89 · Pull Request #2322

Honor select disposition in transactions by slorello89 · Pull Request #2322 · StackExchange/StackExchange.Redis

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

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.

mgravell deleted the slorello89/honor-select-disposition-transactions branch

December 21, 2023 15:04

NickCraver added a commit that referenced this pull request

Jan 16, 2024