Skip to content

Conversation

mykaul
Copy link

@mykaul mykaul commented Aug 7, 2025

While trying to look at some random (flaky?) test (#510 (comment) ) I saw some (minor) improvements that can be made to make_token_replica_map():

  1. Remove some redundant len() calls to outside the loop(s)
  2. Align some variable names, start with num_ ... for them.
  3. Move token_offset and host assignment within the loop to closer to where it's used.
  4. Only add hosts that are in the dc_rf_map to begin with.

item 1 above was also implemented for SimpleStrategy's make_token_replica_map()

All those are probably very very minor improvements, perhaps in a large cluster it'll be noticable.

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.

@mykaul mykaul requested a review from Copilot August 7, 2025 09:14
@mykaul mykaul marked this pull request as draft August 7, 2025 09:14
Copilot

This comment was marked as outdated.

@dkropachev dkropachev marked this pull request as ready for review August 7, 2025 17:52
@dkropachev
Copy link
Collaborator

@mykaul , let me know when you done, it looks good as is.

@mykaul
Copy link
Author

mykaul commented Aug 7, 2025

@mykaul , let me know when you done, it looks good as is.

I'm not, I'd like to understand the code better. I think it can be further slightly improved.

@mykaul mykaul force-pushed the make_token_replica_map branch from cd57c4d to 64c0e38 Compare August 8, 2025 06:23
@mykaul mykaul requested a review from Copilot August 8, 2025 06:25
Copilot

This comment was marked as outdated.

@mykaul
Copy link
Author

mykaul commented Aug 8, 2025

@scylladb/python-driver-maint - please re-review. I hope I did not mess things up - CoPilot is being silly here, I suspect. I'm not sure why the whole CI is broken, doesn't seem to be caused by my changes.
Please review carefully, I don't think we have enough tests for this part, though it seems OK. Perhaps I can just do the removal of the len() stuff outside the loop. It's low impact anyway.

mykaul added 2 commits August 11, 2025 12:28
…ake_token_replica_map()

While trying to look at some random (flaky?) test (scylladb#510 (comment) )
I saw some (minor) improvements that can be made to NetworkTopologyStrategy's make_token_replica_map():
1. Remove some redundant len() calls to outside the loop(s)
2. Align some variable names, start with num_ ... for them.
3. Move token_offset and host assignment within the loop to closer to where it's used.
4. Only add DCs and hosts that are in the map

All those are probably very very minor improvements, perhaps in a large cluster it'll be noticable.

Signed-off-by: Yaniv Kaul <[email protected]>
…_replica_map()

While trying to look at some random (flaky?) test (scylladb#510 (comment) )
I saw some (minor) improvements that can be made to SimpleStrategy's make_token_replica_map():
Specifically - remove some redundant len() calls to outside the loop(s)

Probably very very minor improvement, perhaps in a large cluster it'll be noticable - but no one should use SimpleStrategy anyway!

Signed-off-by: Yaniv Kaul <[email protected]>
@mykaul mykaul force-pushed the make_token_replica_map branch from 64c0e38 to b9e5d44 Compare August 11, 2025 09:31
@mykaul mykaul requested a review from Copilot August 11, 2025 10:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces performance optimizations to the make_token_replica_map() method by reducing redundant computations and improving variable naming consistency. The changes focus on moving length calculations outside loops and filtering hosts based on datacenter replication factor upfront.

  • Cache len() calculations outside loops to avoid repeated computations
  • Rename variables to use consistent num_ prefix for count-related variables
  • Filter hosts by datacenter replication factor earlier in the process

@dkropachev dkropachev merged commit cad1e91 into scylladb:master Aug 11, 2025
18 of 19 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.

3 participants