-
Notifications
You must be signed in to change notification settings - Fork 3
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
added get_replica_connections function #72
base: main
Are you sure you want to change the base?
Changes from 8 commits
e7beff6
6a717ab
0791c7e
c77580e
5ee68b7
c4dc33a
2c20a1e
99b6671
dd8a4ad
8eed87f
e646320
4a3d585
a1262e0
199a1e5
71622fe
9f261fc
9d21e95
6aad7bd
2a96be0
b693987
519b9dd
557f04f
222cfb4
a9f597e
ac8471e
cfd0864
65e41d0
d112322
ad7cedd
ac4cfa4
5a444db
4cc0b6a
51d4d90
cb8cd2e
1ad3162
4d4c1ac
263e8bf
af4f748
b1d6c41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -75,6 +75,7 @@ | |||||||||||||||||||||||||||||||||||||||||
dynamic_startup_nodes=True, | ||||||||||||||||||||||||||||||||||||||||||
url=None, | ||||||||||||||||||||||||||||||||||||||||||
address_remap=None, | ||||||||||||||||||||||||||||||||||||||||||
decode_responses=True | ||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
conn = redis.Redis( | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -92,7 +93,7 @@ | |||||||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -125,6 +126,7 @@ | |||||||||||||||||||||||||||||||||||||||||
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,40 @@ | |||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||||||
mode = self.connection.execute_command("info")['redis_mode'] | ||||||||||||||||||||||||||||||||||||||||||
except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||
raise Exception(f"Failed to get Redis mode: {e}") | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve exception handling The current exception handling is too broad and lacks proper exception chaining. Apply this diff: try:
- mode = self.connection.execute_command("info")['redis_mode']
- except Exception as e:
- raise Exception(f"Failed to get Redis mode: {e}")
+ info = self.connection.execute_command("info", section="server")
+ mode = info.get('redis_mode')
+ if not mode:
+ raise ConnectionError("Unable to determine Redis mode")
+ except redis.RedisError as e:
+ raise ConnectionError("Failed to retrieve Redis mode") from e 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 248-251: falkordb/falkordb.py#L248-L251 🪛 Ruff251-251: Within an (B904) |
||||||||||||||||||||||||||||||||||||||||||
if hasattr(self, 'sentinel') and self.sentinel is not None: | ||||||||||||||||||||||||||||||||||||||||||
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, int(port)) for host, port in replica_hostnames] | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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']
|
||||||||||||||||||||||||||||||||||||||||||
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"]] | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 253-258: falkordb/falkordb.py#L253-L258 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that 'ip_port.split(':')' results in a list with at least two elements to avoid an IndexError.
Suggested change
Copilot is powered by AI, so mistakes are possible. Review output carefully before use. |
||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||
raise ValueError(f"Unsupported Redis mode: {mode}") | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Missing test coverage for The method requires test coverage for:
Example test structure in 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 chainAdd test coverage for the new method The new Would you like me to help create comprehensive test cases covering:
🏁 Scripts executedThe 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 [warning] 253-258: falkordb/falkordb.py#L253-L258 [warning] 260-260: falkordb/falkordb.py#L260 🪛 Ruff251-251: Within an (B904) |
||||||||||||||||||||||||||||||||||||||||||
def config_set(self, name: str, value=None) -> None: | ||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||
Update a DB level configuration. | ||||||||||||||||||||||||||||||||||||||||||
|
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.
if
hasattr(self, 'sentinel')
is true, then performing this action is a waste of time.