Skip to content

[GROW-3598] make initialize to loop through replica nodes per cluster slots row #17

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 14, 2023
Merged

[GROW-3598] make initialize to loop through replica nodes per cluster slots row #17

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions redis/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,25 +1561,25 @@ def initialize(self):
# add this node to the nodes cache
tmp_nodes_cache[target_node.name] = target_node

target_replica_nodes = []
for replica_node in slot[3:]:
host = str_if_bytes(replica_node[0])
port = int(replica_node[1])
host, port = self.remap_host_port(host, port)

target_replica_node = self._get_or_create_cluster_node(
host, port, REPLICA, tmp_nodes_cache
)
target_replica_nodes.append(target_replica_node)
# add this node to the nodes cache
tmp_nodes_cache[target_replica_node.name] = target_replica_node

for i in range(int(slot[0]), int(slot[1]) + 1):
if i not in tmp_slots:
tmp_slots[i] = []
tmp_slots[i].append(target_node)
replica_nodes = [slot[j] for j in range(3, len(slot))]
Copy link

Choose a reason for hiding this comment

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

replica_nodes = slot[3:]

if we change this line like above, how much does performance change?

Copy link

@bellatoris bellatoris Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

because all below lines should not affect performance much, I believe. All of its time complexity is O(1)

Copy link
Author

Choose a reason for hiding this comment

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

replica_nodes = slot[3:] improves the time spent up to 11.043858208024176

the bigger difference came from not iterating through replica nodes by the number of slots

Copy link

Choose a reason for hiding this comment

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

i know your change is more neat, just curious for python performance

Copy link

@bellatoris bellatoris Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the bigger difference came from not iterating through replica nodes by the number of slots

Hmm? but aren't replica nodes traversed by the number of slots still in your change?

redis-py/redis/cluster.py

Lines 1577 to 1582 in 2e8fa0c

for i in range(int(slot[0]), int(slot[1]) + 1):
if i not in tmp_slots:
tmp_slots[i] = []
tmp_slots[i].append(target_node)
for target_replica_node in target_replica_nodes:
tmp_slots[i].append(target_replica_node)

Copy link
Author

Choose a reason for hiding this comment

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

that part is true, but looking for node doesn't have to be done in those iterations

host = str_if_bytes(replica_node[0])
port = replica_node[1]
host, port = self.remap_host_port(host, port)

target_replica_node = self._get_or_create_cluster_node(host, port, REPLICA, tmp_nodes_cache)

Copy link

Choose a reason for hiding this comment

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

🤔 🤔 🤔 What's wrong with you python. why the 16384 * above operations bring that much performance difference 🐍


for replica_node in replica_nodes:
host = str_if_bytes(replica_node[0])
port = replica_node[1]
host, port = self.remap_host_port(host, port)

target_replica_node = self._get_or_create_cluster_node(
host, port, REPLICA, tmp_nodes_cache
)
for target_replica_node in target_replica_nodes:
tmp_slots[i].append(target_replica_node)
# add this node to the nodes cache
tmp_nodes_cache[
target_replica_node.name
] = target_replica_node
else:
# Validate that 2 nodes want to use the same slot cache
# setup
Expand Down