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.

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

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
+432 −98
Open

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

Show file tree
Hide file tree
Changes from 2 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
View file
Open in desktop
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?


2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
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/redis/redis-py", rev = "master"}
Copy link

@coderabbitai coderabbitai bot Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Consider the implications of using the master branch for the Redis dependency.

Switching the Redis dependency to the master branch of its Git repository can introduce instability due to the inclusion of potentially untested changes. It's generally safer to depend on a specific release or a stable branch to ensure compatibility and stability of the application.

Consider using a tagged release or a stable branch instead of the master branch:

-redis = {git = "https://github.com/redis/redis-py", rev = "master"}
+redis = {git = "https://github.com/redis/redis-py", rev = "stable"}

Committable suggestion was skipped due to low confidence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to wait for an official release from redis.

Copy link

Choose a reason for hiding this comment

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

@MuhammadQadora, understood! Once the official release from Redis is available, we can revisit the dependency to ensure stability. Let me know if you need any further assistance in the meantime!


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