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

Allow SPUBLISH command within multi/exec on replica #12944

Closed
wants to merge 2 commits into from

Conversation

hpatro
Copy link
Collaborator

@hpatro hpatro commented Jan 12, 2024

Allow SPUBLISH command within multi/exec on replica

Behavior on unstable:

127.0.0.1:6380> CLUSTER NODES
39ce8aa20f1f0d91f1a88d976ee1926dfefcdf1a 127.0.0.1:6380@16380 myself,slave 8b0feb120b68aac489d6a5af9c77dc40d71bc792 0 0 0 connected
8b0feb120b68aac489d6a5af9c77dc40d71bc792 127.0.0.1:6379@16379 master - 0 1705091681202 0 connected 0-16383
127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
(error) MOVED 866 127.0.0.1:6379

With this change:

127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
1) (integer) 0


/* Only valid for sharded pubsub as regular pubsub can operate on any node and bypasses this layer. */
int is_pubsubshard = (cmd_flags & CMD_PUBSUB) ||
(c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_PUBSUB));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two issues:

  1. Whenever a transaction contains shard pubsub commands, the entire transaction is considered as is_pubsubshard, which is too broad of a scope. Although it may not have any practical impact at the moment, there is a potential risk. Perhaps we should rename the variable to include_pubsubshard.
  2. Non-shard pubsub commands, such as publish/subscribe, are also marked as CMD_PUBSUB, which leads to incorrect determination of publish/subscribe in transaction as is_pubsubshard. Although it may not have any practical impact currently, there is a potential risk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @soloestoy.

Agree for point 1, will update it.
I did think about point hence the comment. Explicit command matching across multi command would be slightly painful and introducing a new command type felt unnecessary at this point. Do you think it's reasonable to continue with the current implementation.

@soloestoy soloestoy added the release-notes indication that this issue needs to be mentioned in the release notes label Jan 17, 2024
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sundb
Copy link
Collaborator

sundb commented May 17, 2024

A new PR has been created based on the commits from this PR. Please refer to #13276 for the latest updates and discussion.

sundb added a commit that referenced this pull request May 21, 2024
…ommand (#13276)

This PR is based on the commits from PR #12944.

Allow SPUBLISH command within multi/exec on replica

Behavior on unstable:

```
127.0.0.1:6380> CLUSTER NODES
39ce8aa20f1f0d91f1a88d976ee1926dfefcdf1a 127.0.0.1:6380@16380 myself,slave 8b0feb120b68aac489d6a5af9c77dc40d71bc792 0 0 0 connected
8b0feb120b68aac489d6a5af9c77dc40d71bc792 127.0.0.1:6379@16379 master - 0 1705091681202 0 connected 0-16383
127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
(error) MOVED 866 127.0.0.1:6379
```

With this change:

```
127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
1) (integer) 0
```

---------

Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: oranagra <[email protected]>
filipecosta90 pushed a commit to filipecosta90/redis that referenced this pull request May 28, 2024
…ommand (redis#13276)

This PR is based on the commits from PR redis#12944.

Allow SPUBLISH command within multi/exec on replica

Behavior on unstable:

```
127.0.0.1:6380> CLUSTER NODES
39ce8aa20f1f0d91f1a88d976ee1926dfefcdf1a 127.0.0.1:6380@16380 myself,slave 8b0feb120b68aac489d6a5af9c77dc40d71bc792 0 0 0 connected
8b0feb120b68aac489d6a5af9c77dc40d71bc792 127.0.0.1:6379@16379 master - 0 1705091681202 0 connected 0-16383
127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
(error) MOVED 866 127.0.0.1:6379
```

With this change:

```
127.0.0.1:6380> SPUBLISH hello world
(integer) 0
127.0.0.1:6380> MULTI
OK
127.0.0.1:6380(TX)> SPUBLISH hello world
QUEUED
127.0.0.1:6380(TX)> EXEC
1) (integer) 0
```

---------

Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: oranagra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants