Skip to content

Conversation

@dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Nov 3, 2025

End result should be like this:

  1. Replicas from the same RACK (if rack is specified)
  2. Replicas from the same DC, but Remote RACK (if rack is specified)
  3. Replicas from the same DC (if rack is not specified)
  4. Replicas from the remote DC
  5. Non-replicas in the same order

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@dkropachev dkropachev marked this pull request as ready for review November 3, 2025 17:35
@mykaul
Copy link

mykaul commented Nov 3, 2025

Shouldn't we add tests to ensure this works well?

@wprzytula wprzytula removed their request for review November 4, 2025 13:58
@dkropachev dkropachev force-pushed the dk/fix-tokenaware-routing branch from 1c32d98 to f8b6883 Compare November 4, 2025 16:36
@dkropachev
Copy link
Collaborator Author

Shouldn't we add tests to ensure this works well?

Of course.

@dkropachev dkropachev force-pushed the dk/fix-tokenaware-routing branch from f8b6883 to 95db675 Compare November 4, 2025 17:12
@wprzytula wprzytula assigned wprzytula and dkropachev and unassigned wprzytula Nov 4, 2025
@dkropachev dkropachev force-pushed the dk/fix-tokenaware-routing branch 2 times, most recently from 159db98 to 1841776 Compare November 4, 2025 18:32
@sylwiaszunejko
Copy link
Collaborator

Replicas from the same RACK (if rack is specified)
Replicas from the same DC, but Remote RACK (if rack is specified)
Replicas from the same DC (if rack is not specified)
Replicas from the remote DC
Non-replicas in the same order

Are we sure that this is the right order? I vaguely remember a discussion during summit whether we should query local (dc/rack) nodes that are non-replicas or remote replicas first, and the answer was not obvious. @Lorak-mmk @avikivity

@dkropachev
Copy link
Collaborator Author

@gusev-p we would appreciate your opinion on this matter too.

@dkropachev
Copy link
Collaborator Author

@gusev-p , @avikivity , @Lorak-mmk , please take a look, we need to release python-driver ASAP

Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

I don't see a point in waiting for discussion about the correct order. The PR improves the order undoubtedly, and it matches e.g. Rust Driver order (and probably some other drivers too).
Changing this order is imo a separate task and we can discuss it in https://github.com/scylladb/scylla-drivers/issues/36

End result should be like this:
1. Replicas from the same RACK (if rack is specified)
2. Replicas from the same DC, but Remote RACK (if rack is specified)
3. Replicas from the same DC (if rack is not specified)
3. Replicas from the remote DC
5. Non-replicas in the same order
@dkropachev dkropachev force-pushed the dk/fix-tokenaware-routing branch from 1841776 to af7e1dd Compare November 5, 2025 17:26
@dkropachev dkropachev requested a review from gusev-p November 5, 2025 17:27
@dkropachev dkropachev merged commit 2f271be into master Nov 5, 2025
17 checks passed
@dkropachev dkropachev deleted the dk/fix-tokenaware-routing branch November 5, 2025 17:41
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.

7 participants