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

CassandraSinkCluster: Fix system.peers rpc_address #837

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Oct 4, 2022

rpc_address is a field in system.peers but not system.peers_v2
Driver's often still use system.peers instead of system.peers_v2 for routing.
So this field turned out to actually be important but we were just setting it to null.

As expected I had to change rpc_address fields in the integration tests.
However I also had to change the system.local listen_address in cluster_multi_rack, I assume that this change is now allowing the driver to properly route between multiple shotover instances resulting in different listen_address values.

@rukai rukai force-pushed the fix_system_local_rpc_address branch from b20ca76 to 7b7db90 Compare October 4, 2022 01:44
@rukai rukai force-pushed the fix_system_local_rpc_address branch from 7b7db90 to 536bf9a Compare October 4, 2022 02:01
@rukai rukai marked this pull request as ready for review October 4, 2022 02:39
@rukai rukai changed the title CassandraSinkCluster: Fix system.local rpc_address CassandraSinkCluster: Fix system.peers rpc_address Oct 4, 2022
@benbromhead
Copy link
Member

I feel like we should have an integration test that catches this / demonstrates the fix. Or is it the case that our test C* drivers don't hit this edge case?

Copy link
Member

@benbromhead benbromhead left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm

@rukai
Copy link
Member Author

rukai commented Oct 5, 2022

The integration tests do catch it, we just have to use Result::Any because we dont know which node we are talking to.
Maybe we should rewrite our assertions to assert that we receive 2 of 3 expected results.

@rukai rukai merged commit 62a3fb5 into shotover:main Oct 5, 2022
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.

3 participants