Skip to content

added get_replica_connections function #72

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

Open
MuhammadQadora wants to merge 39 commits into
base: main
Choose a base branch
from
Open

added get_replica_connections function #72

Changes from 6 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
e7beff6
added get_replica_connections function
MuhammadQadora Oct 28, 2024
6a717ab
only return connections
MuhammadQadora Nov 6, 2024
0791c7e
removed flag
MuhammadQadora Nov 14, 2024
c77580e
updated to suite Coderabbit comments
MuhammadQadora Nov 14, 2024
5ee68b7
fixed function
MuhammadQadora Nov 14, 2024
c4dc33a
removed unnecessary variable
MuhammadQadora Nov 14, 2024
2c20a1e
Update falkordb.py
swilly22 Nov 14, 2024
99b6671
added error handling
MuhammadQadora Nov 14, 2024
dd8a4ad
removed error handler
MuhammadQadora Nov 14, 2024
8eed87f
added test
MuhammadQadora Nov 18, 2024
e646320
added tests
MuhammadQadora Nov 20, 2024
4a3d585
fixed path
MuhammadQadora Nov 20, 2024
a1262e0
updated
MuhammadQadora Nov 20, 2024
199a1e5
updated
MuhammadQadora Nov 20, 2024
71622fe
updated test
MuhammadQadora Nov 21, 2024
9f261fc
test for clusterCon and SentinelCon
MuhammadQadora Nov 21, 2024
9d21e95
removed wrong import
MuhammadQadora Nov 21, 2024
6aad7bd
fixed test
MuhammadQadora Nov 21, 2024
2a96be0
updated test
MuhammadQadora Nov 22, 2024
b693987
changed
MuhammadQadora Nov 24, 2024
519b9dd
updated
MuhammadQadora Nov 24, 2024
557f04f
added compose files
MuhammadQadora Dec 2, 2024
222cfb4
created deployments standalone, redis cluster and redis replication f…
MuhammadQadora Dec 16, 2024
a9f597e
testing
MuhammadQadora Dec 18, 2024
ac8471e
created deployments standalone, redis cluster and redis replication f…
MuhammadQadora Dec 19, 2024
cfd0864
created deployments standalone, redis cluster and redis replication f…
MuhammadQadora Dec 19, 2024
65e41d0
full path
MuhammadQadora Dec 19, 2024
d112322
removed exceptions
MuhammadQadora Dec 19, 2024
ad7cedd
moved docker files to docker folder
swilly22 Dec 22, 2024
ac4cfa4
changed image to use falkordb/falkordb:edge and overriden the entrypoint
MuhammadQadora Dec 26, 2024
5a444db
fixed entrypoint
MuhammadQadora Dec 26, 2024
4cc0b6a
added yml suffix
MuhammadQadora Dec 26, 2024
51d4d90
updated image tag
MuhammadQadora Dec 29, 2024
cb8cd2e
[WIP] improved over all test latency
swilly22 Dec 29, 2024
1ad3162
Merge branch 'main' of https://github.com/FalkorDB/falkordb-py into 7…
MuhammadQadora Dec 30, 2024
4d4c1ac
change compose files and removed the need for docker compose down
MuhammadQadora Jan 1, 2025
263e8bf
change compose files and removed the need for docker compose down
MuhammadQadora Jan 2, 2025
af4f748
removed echos
MuhammadQadora Jan 2, 2025
b1d6c41
enhanced waiting
MuhammadQadora Jan 2, 2025
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
35 changes: 34 additions & 1 deletion falkordb/falkordb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ def __init__(
dynamic_startup_nodes=True,
url=None,
address_remap=None,
decode_responses=True
):

conn = redis.Redis(
Expand All @@ -92,7 +93,7 @@ def __init__(
encoding_errors=encoding_errors,
charset=charset,
errors=errors,
decode_responses=True,
decode_responses=decode_responses,
retry_on_timeout=retry_on_timeout,
retry_on_error=retry_on_error,
ssl=ssl,
Expand Down Expand Up @@ -125,6 +126,7 @@ def __init__(
self.sentinel, self.service_name = Sentinel_Conn(conn, ssl)
Comment on lines 174 to 125
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid wildcard imports to improve code clarity

Using wildcard imports can pollute the namespace and make it harder to track dependencies. Explicit imports enhance code readability and maintainability.

🧰 Tools
🪛 Ruff

174-174: Is_Sentinel may be undefined, or defined from star imports

(F405)

175-175: Sentinel_Conn may be undefined, or defined from star imports

(F405)

⚠️ Potential issue

Import Is_Sentinel and Sentinel_Conn explicitly to avoid undefined names

The functions Is_Sentinel and Sentinel_Conn may be undefined if not imported properly. Wildcard imports (e.g., from .sentinel import *) can lead to unclear code and potential NameErrors.

Apply this diff to fix the imports:

-from .sentinel import *
+from .sentinel import Is_Sentinel, Sentinel_Conn

Similarly, update the cluster imports:

-from .cluster import *
+from .cluster import Is_Cluster, Cluster_Conn

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Ruff

174-174: Is_Sentinel may be undefined, or defined from star imports

(F405)

175-175: Sentinel_Conn may be undefined, or defined from star imports

(F405)

conn = self.sentinel.master_for(self.service_name, ssl=ssl)


if Is_Cluster(conn):
conn = Cluster_Conn(
conn,
Expand Down Expand Up @@ -225,6 +227,37 @@ def config_get(self, name: str) -> Union[int, str]:

return self.connection.execute_command(CONFIG_CMD, "GET", name)[1]


def get_replica_connections(self):
"""
Retrieve a list of connections for Redis replica nodes.

This function determines the Redis setup (Sentinel or Cluster) and returns
the hostnames and ports of the replicas.

Returns:
list of tuple: A list of (hostname, port) tuples representing the
replica connections.

Raises:
ConnectionError: If unable to connect or retrieve information from
the Redis setup.
ValueError: If the `redis_mode` is neither Sentinel nor Cluster.
"""
# decide if it's Sentinel or cluster
redis_mode = self.connection.execute_command("info")['redis_mode']
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for Redis info command

The Redis info command could fail due to connection issues. Add proper error handling:

-        redis_mode = self.connection.execute_command("info")['redis_mode']
+        try:
+            info = self.connection.execute_command("info", section="server")
+            redis_mode = info.get('redis_mode')
+            if not redis_mode:
+                raise ConnectionError("Unable to determine Redis mode")
+        except Exception as e:
+            raise ConnectionError(f"Failed to retrieve Redis mode: {str(e)}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
redis_mode = self.connection.execute_command("info")['redis_mode']
try:
info = self.connection.execute_command("info", section="server")
redis_mode = info.get('redis_mode')
if not redis_mode:
raise ConnectionError("Unable to determine Redis mode")
except Exception as e:
raise ConnectionError(f"Failed to retrieve Redis mode: {str(e)}")

if hasattr(self, 'sentinel') and self.sentinel is not None:
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update deprecated Redis terminology

The code uses deprecated 'slave' terminology. Redis has moved to more inclusive language.

Update the terminology:

-            replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
+            replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)

-            return [(flag['hostname'], int(ip_port.split(':')[1])) for ip_port, flag in data.items() if 'slave' in flag["flags"] and flag["hostname"]]
+            return [(flag.get('hostname', ip_port.split(':')[0]), int(ip_port.split(':')[1])) 
+                    for ip_port, flag in data.items() 
+                    if 'replica' in flag.get('flags', [])]

Also applies to: 255-255

return [(host, port) for host, port in replica_hostnames]
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update implementation to use modern Redis terminology and handle edge cases

The current implementation has several issues:

  1. Uses deprecated 'slave' terminology
  2. Assumes 'hostname' exists in cluster node flags
  3. Doesn't convert port to integer
-            replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
-            return [(host, port) for host, port in replica_hostnames]
+            try:
+                replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)
+                return [(host, int(port)) for host, port in replica_hostnames]
+            except Exception as e:
+                raise ConnectionError(f"Failed to discover replicas: {str(e)}")

# For cluster mode:
-            return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
+            result = []
+            for ip_port, flag in data.items():
+                if 'replica' in flag.get('flags', []):
+                    try:
+                        host = ip_port.split(':')[0]  # Use IP as hostname
+                        port = int(ip_port.split(':')[1])
+                        result.append((host, port))
+                    except (IndexError, ValueError) as e:
+                        continue  # Skip malformed entries
+            return result

Also applies to: 254-255

elif redis_mode == "cluster":
data = self.connection.cluster_nodes()
# List comprehension to get a list of (hostname, port) tuples
return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential KeyError in cluster mode implementation

The current implementation assumes the presence of a 'hostname' key and doesn't handle potential errors.

Apply this diff:

-            return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
+            result = []
+            for ip_port, flag in data.items():
+                if 'replica' in flag.get('flags', []):  # Using 'replica' instead of 'slave'
+                    try:
+                        host = ip_port.split(':')[0]  # Use IP as hostname
+                        port = int(ip_port.split(':')[1])
+                        result.append((host, port))
+                    except (IndexError, ValueError) as e:
+                        continue  # Skip malformed entries
+            return result
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
result = []
for ip_port, flag in data.items():
if 'replica' in flag.get('flags', []): # Using 'replica' instead of 'slave'
try:
host = ip_port.split(':')[0] # Use IP as hostname
port = int(ip_port.split(':')[1])
result.append((host, port))
except (IndexError, ValueError) as e:
continue # Skip malformed entries
return result
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 248-254: falkordb/falkordb.py#L248-L254
Added lines #L248 - L254 were not covered by tests

else:
raise ValueError(f"Unsupported Redis mode: {redis_mode}")
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix undefined variable reference

The error message references redis_mode but the variable is named mode.

Apply this diff:

-            raise ValueError(f"Unsupported Redis mode: {redis_mode}")
+            raise ValueError(f"Unsupported Redis mode: {mode}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise ValueError(f"Unsupported Redis mode: {redis_mode}")
raise ValueError(f"Unsupported Redis mode: {mode}")
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 256-256: falkordb/falkordb.py#L256
Added line #L256 was not covered by tests

🪛 Ruff

256-256: redis_mode may be undefined, or defined from star imports

(F405)




Comment on lines +231 to +261
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing test coverage for get_replica_connections method confirmed

The method requires test coverage for:

  • Sentinel mode replica discovery
  • Cluster mode replica discovery
  • Error handling for connection failures
  • Invalid mode handling

Example test structure in tests/test_db.py:

def test_get_replica_connections_sentinel():
    # Test sentinel mode with mocked sentinel.discover_slaves

def test_get_replica_connections_cluster():
    # Test cluster mode with mocked cluster_nodes

def test_get_replica_connections_errors():
    # Test connection errors and invalid modes
🔗 Analysis chain

Add test coverage for the new method

The new get_replica_connections method lacks test coverage for critical paths.

Would you like me to help create comprehensive test cases covering:

  1. Sentinel mode replica discovery
  2. Cluster mode replica discovery
  3. Error handling scenarios
  4. Invalid mode handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for existing tests of the new method
fd test_.+\.py$ | xargs rg "get_replica_connections"

Length of output: 53

Script:

#!/bin/bash
# Let's first check if there are any test files in the repository
fd "test.*\.py$"

# Then let's check the content of the falkordb.py file to understand the test requirements better
cat falkordb/falkordb.py

Length of output: 9548

Script:

#!/bin/bash
# Let's check test_db.py as it's likely to contain tests for FalkorDB class methods
cat tests/test_db.py

# Also check if there are any mock or fixture utilities for testing
fd "conftest\.py$"

Length of output: 1366

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 248-251: falkordb/falkordb.py#L248-L251
Added lines #L248 - L251 were not covered by tests

[warning] 253-258: falkordb/falkordb.py#L253-L258
Added lines #L253 - L258 were not covered by tests

[warning] 260-260: falkordb/falkordb.py#L260
Added line #L260 was not covered by tests

🪛 Ruff

251-251: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

def config_set(self, name: str, value=None) -> None:
"""
Update a DB level configuration.
Expand Down