added get_replica_connections function #72
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -75,6 +75,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||||||
| dynamic_startup_nodes=True, | ||||||||||||||||||||||||||||||||||||||||||
| url=None, | ||||||||||||||||||||||||||||||||||||||||||
| address_remap=None, | ||||||||||||||||||||||||||||||||||||||||||
| decode_responses=True | ||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| conn = redis.Redis( | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -92,7 +93,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||||||
| encoding_errors=encoding_errors, | ||||||||||||||||||||||||||||||||||||||||||
| charset=charset, | ||||||||||||||||||||||||||||||||||||||||||
| errors=errors, | ||||||||||||||||||||||||||||||||||||||||||
| decode_responses=decode_responses, | ||||||||||||||||||||||||||||||||||||||||||
| retry_on_timeout=retry_on_timeout, | ||||||||||||||||||||||||||||||||||||||||||
| retry_on_error=retry_on_error, | ||||||||||||||||||||||||||||||||||||||||||
| ssl=ssl, | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -125,6 +126,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||||||
| self.sentinel, self.service_name = Sentinel_Conn(conn, ssl) | ||||||||||||||||||||||||||||||||||||||||||
| conn = self.sentinel.master_for(self.service_name, ssl=ssl) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if Is_Cluster(conn): | ||||||||||||||||||||||||||||||||||||||||||
| conn = Cluster_Conn( | ||||||||||||||||||||||||||||||||||||||||||
| conn, | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 FalkorDB replicas. | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| This function determines the FalkorDB 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 FalkorDB setup. | ||||||||||||||||||||||||||||||||||||||||||
| ValueError: If the `mode` is neither Sentinel nor Cluster. | ||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||
| # decide if it's Sentinel or cluster | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| mode = self.connection.execute_command("info")['redis_mode'] | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
| mode = self.connection.execute_command("info")['redis_mode'] | |
| mode = self.connection.execute_command("info").get('redis_mode', None) |
Outdated
There was a problem hiding this comment.
🛠️ 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
Outdated
There was a problem hiding this comment.
Update deprecated Redis terminology
The code uses deprecated 'slave' terminology. Redis has moved to more inclusive language.
Apply this diff:
- 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: 258-258
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize control flow for Sentinel mode
The current implementation checks Redis mode even when we already know it's in Sentinel mode. This is inefficient.
Modify the flow to prioritize Sentinel check:
- mode = self.connection.execute_command("info")['redis_mode']
- if hasattr(self, 'sentinel') and self.sentinel is not None:
+ if hasattr(self, 'sentinel') and self.sentinel is not None:
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
return [(host, int(port)) for host, port in replica_hostnames]
+
+ mode = self.connection.execute_command("info")['redis_mode']Committable suggestion skipped: line range outside the PR's diff.
Outdated
There was a problem hiding this comment.
Add error handling for cluster mode
The cluster mode implementation lacks proper error handling for malformed data.
Apply this diff:
elif mode == "cluster":
- data = self.connection.cluster_nodes()
- return [(flag['hostname'], int(ip_port.split(':')[1])) for ip_port, flag in data.items() if 'slave' in flag["flags"] and flag["hostname"]]
+ try:
+ data = self.connection.cluster_nodes()
+ result = []
+ for ip_port, flag in data.items():
+ if 'replica' in flag.get('flags', []):
+ try:
+ host = flag.get('hostname', ip_port.split(':')[0])
+ port = int(ip_port.split(':')[1])
+ result.append((host, port))
+ except (IndexError, ValueError) as e:
+ continue # Skip malformed entries
+ return result
+ except redis.RedisError as e:
+ raise ConnectionError("Failed to retrieve cluster nodes") from 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.
| elif mode == "cluster": | |
| data = self.connection.cluster_nodes() | |
| return [(flag['hostname'], int(ip_port.split(':')[1])) for ip_port, flag in data.items() if 'slave' in flag["flags"] and flag["hostname"]] | |
| elif mode == "cluster": | |
| try: | |
| data = self.connection.cluster_nodes() | |
| result = [] | |
| for ip_port, flag in data.items(): | |
| if 'replica' in flag.get('flags', []): | |
| try: | |
| host = flag.get('hostname', ip_port.split(':')[0]) | |
| port = int(ip_port.split(':')[1]) | |
| result.append((host, port)) | |
| except (IndexError, ValueError) as e: | |
| continue # Skip malformed entries | |
| return result | |
| except redis.RedisError as e: | |
| raise ConnectionError("Failed to retrieve cluster nodes") from e |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 253-258: falkordb/falkordb.py#L253-L258
Added lines #L253 - L258 were not covered by tests
Outdated
There was a problem hiding this comment.
Ensure that 'ip_port.split(':')' results in a list with at least two elements to avoid an IndexError.
| 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['hostname'], int(ip_port.split(':')[1])) for ip_port, flag in data.items() if 'slave' in flag['flags'] and flag['hostname'] and len(ip_port.split(':')) > 1] |
There was a problem hiding this comment.
💡 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:
- Sentinel mode replica discovery
- Cluster mode replica discovery
- Error handling scenarios
- 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)
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.
🛠️ 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
Import
Is_SentinelandSentinel_Connexplicitly to avoid undefined namesThe functions
Is_SentinelandSentinel_Connmay be undefined if not imported properly. Wildcard imports (e.g.,from .sentinel import *) can lead to unclear code and potentialNameErrors.Apply this diff to fix the imports:
Similarly, update the cluster imports:
🧰 Tools
🪛 Ruff