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

57 changing the cluster conn function to use domain nameshostnames instead of ips #58

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
534723e
changed Cluster_Conn to use hostnames instead of ips
MuhammadQadora Sep 5, 2024
eff9ae8
changed redis version to use master until new release
MuhammadQadora Sep 5, 2024
3fd5c14
changed redis dependency to point to Dudi's branch
MuhammadQadora Sep 8, 2024
e5cfe80
for testing
MuhammadQadora Sep 8, 2024
377fae2
lowered retries
MuhammadQadora Sep 8, 2024
eef67b8
updated retry times
MuhammadQadora Sep 8, 2024
e65ee26
lowered retries
MuhammadQadora Sep 9, 2024
97bbe4a
insane retry count
MuhammadQadora Sep 10, 2024
0e79477
changed dependenct to debug
MuhammadQadora Sep 11, 2024
98719f6
reduced retry count
MuhammadQadora Sep 11, 2024
f65ce34
updated dependency
MuhammadQadora Sep 11, 2024
f623557
updated retry attempts
MuhammadQadora Sep 16, 2024
427ef70
Update falkordb.py
MuhammadQadora Sep 17, 2024
523ff53
Update cluster.py
MuhammadQadora Sep 17, 2024
8509151
Update pyproject.toml
MuhammadQadora Sep 17, 2024
09c969a
Update cluster.py
MuhammadQadora Sep 17, 2024
da45121
Update falkordb.py
MuhammadQadora Sep 19, 2024
14580bc
changed dependenc
MuhammadQadora Sep 19, 2024
ac39f29
Merge branch '57-changing-the-cluster_conn-function-to-use-domain-nam…
MuhammadQadora Sep 19, 2024
c074f70
Update falkordb.py
MuhammadQadora Sep 19, 2024
adbbada
Update cluster.py
MuhammadQadora Sep 19, 2024
670e908
changed dependenc
MuhammadQadora Sep 25, 2024
af46b2b
Merge branch '57-changing-the-cluster_conn-function-to-use-domain-nam…
MuhammadQadora Sep 25, 2024
d795189
Update falkordb.py
MuhammadQadora Sep 25, 2024
d05f644
Update falkordb.py
MuhammadQadora Sep 26, 2024
343f0c0
Update falkordb.py
MuhammadQadora Sep 29, 2024
a43db2e
Update falkordb.py
MuhammadQadora Oct 13, 2024
674c651
Update falkordb.py
MuhammadQadora Oct 13, 2024
4c485b4
Update falkordb.py
MuhammadQadora Oct 13, 2024
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
12 changes: 6 additions & 6 deletions falkordb/cluster.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from redis.cluster import RedisCluster
from redis.cluster import RedisCluster, ClusterNode
from redis.retry import Retry
from redis.backoff import default_backoff

# detect if a connection is a sentinel
def Is_Cluster(conn):
Expand All @@ -7,12 +9,10 @@

# create a cluster connection from a Redis connection
def Cluster_Conn(conn, ssl):
# current sentinel
info = conn.execute_command("CLUSTER NODES")
nodes = [ ClusterNode(v['hostname'],k.split(':')[1]) for k,v in info.items()]

Check warning on line 13 in falkordb/cluster.py

View check run for this annotation

Codecov / codecov/patch

falkordb/cluster.py#L12-L13

Added lines #L12 - L13 were not covered by tests
connection_kwargs = conn.connection_pool.connection_kwargs
host = connection_kwargs['host']
port = connection_kwargs['port']
username = connection_kwargs['username']
password = connection_kwargs['password']

return RedisCluster(host=host, port=port, username=username, password=password, ssl=ssl)
return RedisCluster(Retry(default_backoff(),6),cluster_error_retry_attempts=6,startup_nodes=nodes,username=username,password=password,ssl=ssl)

Check warning on line 17 in falkordb/cluster.py

View check run for this annotation

Codecov / codecov/patch

falkordb/cluster.py#L17

Added line #L17 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

Implement retry mechanism in the Cluster_Conn function.

The addition of a retry mechanism using Retry with a default backoff strategy enhances the resilience of the connection process. This is a good practice for distributed systems where transient errors are common. However, ensure that the new lines of code are covered by unit tests to maintain code quality and reliability.

The static analysis tool indicates that these lines are not covered by tests. Consider adding unit tests to cover these new functionalities.

# Example unit test for the retry mechanism
def test_retry_mechanism():
    # Setup test
    # Assert retry behavior
Tools
GitHub Check: codecov/patch

[warning] 17-17: falkordb/cluster.py#L17
Added line #L17 was not covered by tests

Copy link

Choose a reason for hiding this comment

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

Significant improvements in Cluster_Conn but ensure unit tests are added.

The modifications to use the CLUSTER NODES command and dynamically construct ClusterNode instances are well-aligned with the PR objectives. The addition of a retry mechanism with a default backoff strategy enhances the resilience of the connection process.

However, as indicated by static analysis tools and previous comments, these changes are not covered by unit tests. It is crucial to ensure that new functionalities work as expected and do not introduce regressions.

Would you like me to help by generating the unit testing code or opening a GitHub issue to track this task?


257 changes: 257 additions & 0 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ packages = [{include = "falkordb"}]

[tool.poetry.dependencies]
python = "^3.8"
redis = "^5.0.1"
redis = {git = "https://github.com/dudizimber/redis-py", rev = "3342-add-hostname-to-parse-node-line"}

[tool.poetry.group.test.dependencies]
pytest-cov = ">=4.1,<6.0"
Expand Down