-
Notifications
You must be signed in to change notification settings - Fork 48
(minor improvement): small refactoring in make_token_replica_map() #513
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
Conversation
@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. |
cd57c4d
to
64c0e38
Compare
@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. |
…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]>
64c0e38
to
b9e5d44
Compare
There was a problem hiding this 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
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()
: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
./docs/source/
.Fixes:
annotations to PR description.