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

chore: Add FakeRedis capture/compare to more cluster tests and seed during migration #4542

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Feb 2, 2025

Note that we can't add such a test to test_cluster_native_client because we don't do a migration there, just re-assign slots, meaning that data is lost (by desgin?)

Fixes #4429

Note that we can't add such a test to `test_cluster_native_client`
because we don't do a migration there, just re-assign slots, meaning
that data is lost (by desgin?)

Fixes #4429
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

If I remember correctly this is because data sent during migrations would not be captured right ?

@chakaz
Copy link
Collaborator Author

chakaz commented Feb 3, 2025

If I remember correctly this is because data sent during migrations would not be captured right ?

This is just a "free" step, which we compare ourselves to FakeRedis. Sort of sanity check to see that we behave like Redis does.

We're supposed to take this further and seed during migration, then compare to FakeRedis. So far we didn't do it because we didn't have anything to compare against, as traffic sent during migration was only available to the target node once the migration finished.

kostasrim
kostasrim previously approved these changes Feb 3, 2025
@chakaz chakaz changed the title chore: Add FakeRedis capture/compare to more cluster tests chore: Add FakeRedis capture/compare to more cluster tests and seed during migration Feb 3, 2025
@chakaz
Copy link
Collaborator Author

chakaz commented Feb 3, 2025

@kostasrim I now added the remainder of the tests (originally I thought about doing so in a separate PR, but I gave up on that due to time considerations).
If everything passes in CI then this should be the fix to solve the issue.

@chakaz chakaz requested a review from adiholden February 4, 2025 08:34
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.

Change cluster test to stream commands while migration
2 participants