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

Handle SPUBLISH as a non key-based command #182

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

barshaul
Copy link

@barshaul barshaul commented Aug 8, 2024

This change ensures that SPUBLISH messages won’t be redirected to a random node if the intended target node isn’t available. The optimization of routing to a random node should only apply to commands that return a MOVED error if the target node isn’t the slot owner. The issue with SPUBLISH is that its behavior is unpredictable when redirected: if the randomly chosen node is outside the slot’s shard, a MOVED error will be returned; otherwise, the command might succeed, leading to inconsistent behavior. To address this, we will implement a PRIMARY_PREFERRED routing strategy for SPUBLISH, deliberately routing failed SPUBLISH commands to other nodes within the same shard.

@barshaul barshaul requested a review from ikolomi August 8, 2024 13:14
@ikolomi
Copy link

ikolomi commented Aug 8, 2024

I am not convinced that this policy improves and not degrades the current one:
It is actually better to try publishing on the other nodes in an effort to get the message to through.
The state of publisher does not necessary match the state of the listeners - Hence, under the principle that loosing less connections are more likely than loosing more, having the publisher try to use other connections for completing its job is justified.

@barshaul barshaul force-pushed the fix_publish branch 2 times, most recently from 1761625 to c5be729 Compare August 14, 2024 14:13
@barshaul barshaul changed the title Handle PUBLISH as a non key-based command Handle SPUBLISH as a non key-based command Aug 14, 2024
@barshaul barshaul merged commit efd61ff into amazon-contributing:main Aug 14, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants