added get_replica_connections function #72
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to the FalkorDB library's connection management and testing infrastructure. The changes include modifications to the Changes
Sequence DiagramsequenceDiagram
participant Client as FalkorDB Client
participant Redis as Redis Server
participant Sentinel as Redis Sentinel
participant Cluster as Redis Cluster
Client->>Redis: get_replica_connections()
alt Standalone Mode
Redis-->>Client: Raise ValueError
end
Client->>Sentinel: get_replica_connections()
Sentinel-->>Client: Return Replica Connections
Client->>Cluster: get_replica_connections()
Cluster-->>Client: Return Replica Connections
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 92.26% 93.34% +1.07%
==========================================
Files 35 36 +1
Lines 2613 2688 +75
==========================================
+ Hits 2411 2509 +98
+ Misses 202 179 -23 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
falkordb/falkordb.py (1)
177-177: Increase test coverage for new codeThe new lines added are not covered by existing tests. To ensure functionality and prevent future regressions, please add tests covering these lines.
Also applies to: 192-192, 283-285, 334-336, 338-339, 390-390
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 177-177: falkordb/falkordb.py#L177
Added line #L177 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- falkordb/falkordb.py (5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 177-177: falkordb/falkordb.py#L177
Added line #L177 was not covered by tests
[warning] 192-192: falkordb/falkordb.py#L192
Added line #L192 was not covered by tests
[warning] 283-285: falkordb/falkordb.py#L283-L285
Added lines #L283 - L285 were not covered by tests
[warning] 334-336: falkordb/falkordb.py#L334-L336
Added lines #L334 - L336 were not covered by tests
[warning] 338-339: falkordb/falkordb.py#L338-L339
Added lines #L338 - L339 were not covered by tests
[warning] 390-390: falkordb/falkordb.py#L390
Added line #L390 was not covered by tests
🪛 Ruff
falkordb/falkordb.py
174-174:
Is_Sentinelmay be undefined, or defined from star imports(F405)
175-175:
Sentinel_Connmay be undefined, or defined from star imports(F405)
🔇 Additional comments (2)
falkordb/falkordb.py (2)
78-78: LGTM!Adding the
decode_responsesparameter to the__init__method and passing it toredis.Redisallows for flexible response decoding. The implementation is correct.
96-96: LGTM!Passing
decode_responses=decode_responsestoredis.Redisensures that response decoding behavior aligns with the user's preference.
falkordb/falkordb.py
Outdated
| self.password=password | ||
| self.socket_timeout=socket_timeout | ||
| self.socket_connect_timeout=socket_connect_timeout | ||
| self.socket_keepalive=socket_keepalive | ||
| self.socket_keepalive_options=socket_keepalive_options | ||
| self.connection_pool=connection_pool | ||
| self.unix_socket_path=unix_socket_path | ||
| self.encoding=encoding | ||
| self.encoding_errors=encoding_errors | ||
| self.charset=charset | ||
| self.errors=errors | ||
| self.retry_on_timeout=retry_on_timeout | ||
| self.retry_on_error=retry_on_error | ||
| self.ssl=ssl | ||
| self.ssl_keyfile=ssl_keyfile | ||
| self.ssl_certfile=ssl_certfile | ||
| self.ssl_cert_reqs=ssl_cert_reqs | ||
| self.ssl_ca_certs=ssl_ca_certs | ||
| self.ssl_ca_path=ssl_ca_path | ||
| self.ssl_ca_data=ssl_ca_data | ||
| self.ssl_check_hostname=ssl_check_hostname | ||
| self.ssl_password=ssl_password | ||
| self.ssl_validate_ocsp=ssl_validate_ocsp | ||
| self.ssl_validate_ocsp_stapled=ssl_validate_ocsp_stapled | ||
| self.ssl_ocsp_context=ssl_ocsp_context | ||
| self.ssl_ocsp_expected_cert=ssl_ocsp_expected_cert | ||
| self.max_connections=max_connections | ||
| self.single_connection_client=single_connection_client | ||
| self.health_check_interval=health_check_interval | ||
| self.client_name=client_name | ||
| self.lib_name=lib_name | ||
| self.lib_version=lib_version | ||
| self.username=username | ||
| self.retry=retry | ||
| self.connect_func=connect_func | ||
| self.credential_provider=credential_provider | ||
| self.protocol=protocol | ||
| # FalkorDB Cluster Params | ||
| self.cluster_error_retry_attempts=cluster_error_retry_attempts | ||
| self.startup_nodes=startup_nodes | ||
| self.require_full_coverage=require_full_coverage | ||
| self.reinitialize_steps=reinitialize_steps | ||
| self.read_from_replicas=read_from_replicas | ||
| self.dynamic_startup_nodes=dynamic_startup_nodes | ||
| self.url=url | ||
| self.address_remap=address_remap | ||
| self.sentinel_flag = False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in instance variable assignments
The __init__ method contains repetitive assignments of parameters to instance variables. This can be simplified to improve maintainability and readability.
[code_smell]
Consider using a loop to assign these variables dynamically:
for attr in [
'password',
'socket_timeout',
'socket_connect_timeout',
'socket_keepalive',
'socket_keepalive_options',
'connection_pool',
'unix_socket_path',
'encoding',
'encoding_errors',
'charset',
'errors',
'retry_on_timeout',
'retry_on_error',
'ssl',
'ssl_keyfile',
'ssl_certfile',
'ssl_cert_reqs',
'ssl_ca_certs',
'ssl_ca_path',
'ssl_ca_data',
'ssl_check_hostname',
'ssl_password',
'ssl_validate_ocsp',
'ssl_validate_ocsp_stapled',
'ssl_ocsp_context',
'ssl_ocsp_expected_cert',
'max_connections',
'single_connection_client',
'health_check_interval',
'client_name',
'lib_name',
'lib_version',
'username',
'retry',
'connect_func',
'credential_provider',
'protocol',
'cluster_error_retry_attempts',
'startup_nodes',
'require_full_coverage',
'reinitialize_steps',
'read_from_replicas',
'dynamic_startup_nodes',
'url',
'address_remap',
]:
setattr(self, attr, locals()[attr])
falkordb/falkordb.py
Outdated
| if Is_Sentinel(conn): | ||
| self.sentinel, self.service_name = Sentinel_Conn(conn, ssl) |
There was a problem hiding this comment.
🛠️ 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_Sentinelmay be undefined, or defined from star imports(F405)
175-175:
Sentinel_Connmay be undefined, or defined from star imports(F405)
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_ConnSimilarly, update the cluster imports:
-from .cluster import *
+from .cluster import Is_Cluster, Cluster_ConnCommittable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
174-174:
Is_Sentinelmay be undefined, or defined from star imports(F405)
175-175:
Sentinel_Connmay be undefined, or defined from star imports(F405)
falkordb/falkordb.py
Outdated
| result = [FalkorDB( host=host, | ||
| password=self.password, | ||
| socket_timeout=self.socket_timeout, | ||
| socket_connect_timeout=self.socket_connect_timeout, | ||
| socket_keepalive=self.socket_keepalive, | ||
| socket_keepalive_options=self.socket_keepalive_options, | ||
| connection_pool=self.connection_pool, | ||
| unix_socket_path=self.unix_socket_path, | ||
| encoding=self.encoding, | ||
| encoding_errors=self.encoding_errors, | ||
| charset=self.charset, | ||
| errors=self.errors, | ||
| retry_on_timeout=self.retry_on_timeout, | ||
| retry_on_error=self.retry_on_error, | ||
| ssl=self.ssl, | ||
| ssl_keyfile=self.ssl_keyfile, | ||
| ssl_certfile=self.ssl_certfile, | ||
| ssl_cert_reqs=self.ssl_cert_reqs, | ||
| ssl_ca_certs=self.ssl_ca_certs, | ||
| ssl_ca_path=self.ssl_ca_path, | ||
| ssl_ca_data=self.ssl_ca_data, | ||
| ssl_check_hostname=self.ssl_check_hostname, | ||
| ssl_password=self.ssl_password, | ||
| ssl_validate_ocsp=self.ssl_validate_ocsp, | ||
| ssl_validate_ocsp_stapled=self.ssl_validate_ocsp_stapled, | ||
| ssl_ocsp_context=self.ssl_ocsp_context, | ||
| ssl_ocsp_expected_cert=self.ssl_ocsp_expected_cert, | ||
| max_connections=self.max_connections, | ||
| single_connection_client=self.single_connection_client, | ||
| health_check_interval=self.health_check_interval, | ||
| client_name=self.client_name, | ||
| lib_name=self.lib_name, | ||
| lib_version=self.lib_version, | ||
| username=self.username, | ||
| retry=self.retry, | ||
| connect_func=self.connect_func, | ||
| credential_provider=self.credential_provider, | ||
| protocol=self.protocol, | ||
| # FalkorDB Cluster Params | ||
| cluster_error_retry_attempts=self.cluster_error_retry_attempts, | ||
| startup_nodes=self.startup_nodes, | ||
| require_full_coverage=self.require_full_coverage, | ||
| reinitialize_steps=self.reinitialize_steps, | ||
| read_from_replicas=self.read_from_replicas, | ||
| dynamic_startup_nodes=self.dynamic_startup_nodes, | ||
| url=self.url, | ||
| address_remap=self.address_remap, | ||
| port=port) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate duplicate code when creating FalkorDB instances
The code for initializing FalkorDB instances in both Sentinel and Cluster modes is nearly identical. Refactor this into a helper method to reduce duplication.
[code_smell]
For example:
def _create_falkordb_instance(self, host, port):
return FalkorDB(
host=host,
port=port,
password=self.password,
socket_timeout=self.socket_timeout,
# ... all other parameters ...
)Then use this method in both branches.
Also applies to: 339-389
falkordb/falkordb.py
Outdated
| if self.sentinel_flag: | ||
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated terminology from 'slave' to 'replica'
The term 'slave' is deprecated in Redis terminology. Update the code to use 'replica' for clarity and to align with modern best practices.
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)Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 283-285: falkordb/falkordb.py#L283-L285
Added lines #L283 - L285 were not covered by tests
falkordb/falkordb.py
Outdated
| host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], flag['hostname']) for ip_port, flag in data.items() if 'slave' in flag["flags"]] | ||
| result = [FalkorDB( |
There was a problem hiding this comment.
💡 Codebase verification
Potential KeyError when accessing 'hostname' in cluster node flags
The code assumes the presence of a 'hostname' key in the cluster node flags, but this appears to be incorrect. Looking at the code context:
- The code is using Redis's cluster_nodes() method directly, which typically returns node information with 'flags' containing role information ('master'/'slave') but not 'hostname'.
- The hostname should be obtained from the ip_port string itself, which is already being split into IP and port components.
Suggested fix:
# Before (problematic):
host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], flag['hostname']) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
# After (corrected):
host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], ip_port.split(':')[0]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]The code should use the IP address as the hostname since that's what's available in the cluster node information, rather than trying to access a non-existent 'hostname' key.
🔗 Analysis chain
Verify existence of 'hostname' key in cluster node flags
In the list comprehension, flag['hostname'] is accessed, but it's unclear if 'hostname' is a valid key in the flag dictionary returned by cluster_nodes(). This may cause a KeyError.
Run the following script to verify the keys in the cluster nodes:
If 'hostname' is not present, consider using available keys such as 'host' or adjusting the data extraction accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'hostname' exists in cluster node flags.
# Test: Check if 'hostname' key is present. Expected: Should be present in all nodes.
python -c "
import redis
conn = redis.Redis(host='localhost', port=6379)
nodes = conn.cluster_nodes()
for node_id, node_info in nodes.items():
if 'hostname' not in node_info:
print(f'Missing hostname in node {node_id}: {node_info}')
"
Length of output: 340
Script:
#!/bin/bash
# Let's examine the actual implementation of cluster_nodes() and related code
# to understand what data structure it returns
# First, check the implementation of cluster_nodes
rg -A 10 "def cluster_nodes"
# Also check any tests that might reveal the structure
rg -l "cluster_nodes.*test"
# Look for any documentation or type hints
rg "cluster_nodes.*->.*Dict"
Length of output: 91
Script:
#!/bin/bash
# Let's look at the actual code context around where this is being used
fd falkordb.py --exec cat {}
Length of output: 22716
Script:
#!/bin/bash
# Let's check the cluster.py file to understand the structure of cluster_nodes() response
fd cluster.py --exec cat {}
Length of output: 1839
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 338-339: falkordb/falkordb.py#L338-L339
Added lines #L338 - L339 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
falkordb/falkordb.py (4)
6-6: Remove unused importThe
socketmodule is imported but never used in the code.-import socket🧰 Tools
🪛 Ruff
6-6:
socketimported but unusedRemove unused import:
socket(F401)
143-143: Add test coverage for cluster mode initializationThe cluster_flag initialization is not covered by tests. Please add test cases to verify the behavior when connecting in cluster mode.
Would you like me to help generate test cases for cluster mode initialization?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-143: falkordb/falkordb.py#L143
Added line #L143 was not covered by tests
232-232: Add docstring for get_replica_connections methodAdd a docstring describing the method's purpose, return type, and possible exceptions.
def get_replica_connections(self): + """ + Get a list of replica connections from either Sentinel or Cluster setup. + + Returns: + List[Tuple[str, str]]: A list of (host, port) tuples representing replica connections + + Raises: + RedisError: If unable to retrieve replica information + ValueError: If redis_mode is neither 'standalone' nor 'cluster' + """
232-246: Add test coverage for get_replica_connectionsThe new method lacks test coverage. Please add test cases for:
- Standalone mode with Sentinel
- Cluster mode
- Error cases (invalid redis_mode)
Would you like me to help generate comprehensive test cases for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 234-240: falkordb/falkordb.py#L234-L240
Added lines #L234 - L240 were not covered by tests
[warning] 242-244: falkordb/falkordb.py#L242-L244
Added lines #L242 - L244 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(6 hunks)
🧰 Additional context used
🪛 Ruff
falkordb/falkordb.py
6-6: socket imported but unused
Remove unused import: socket
(F401)
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 143-143: falkordb/falkordb.py#L143
Added line #L143 was not covered by tests
[warning] 234-240: falkordb/falkordb.py#L234-L240
Added lines #L234 - L240 were not covered by tests
[warning] 242-244: falkordb/falkordb.py#L242-L244
Added lines #L242 - L244 were not covered by tests
🔇 Additional comments (1)
falkordb/falkordb.py (1)
78-78: LGTM: decode_responses parameter addition
The decode_responses parameter is properly integrated with a sensible default value of True and correctly passed to the Redis constructor.
Also applies to: 96-96
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
falkordb/falkordb.py (1)
231-231: Add docstring to document method behaviorAdd a docstring to explain the method's purpose, parameters, return type, and possible exceptions.
Example:
def get_replica_connections(self): """ Get a list of replica connection details based on the current Redis mode. Returns: List[Tuple[str, int]]: A list of (host, port) tuples for replica nodes Raises: ValueError: If Redis mode is not supported ConnectionError: If unable to retrieve replica information """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 233-239: falkordb/falkordb.py#L233-L239
Added lines #L233 - L239 were not covered by tests
[warning] 241-243: falkordb/falkordb.py#L241-L243
Added lines #L241 - L243 were not covered by tests
🔇 Additional comments (2)
falkordb/falkordb.py (2)
78-78: LGTM: decode_responses parameter addition
The addition of the decode_responses parameter with a default value of True is well-implemented and correctly passed to the Redis constructor.
Also applies to: 96-96
233-243: Add test coverage for get_replica_connections
The method lacks test coverage according to codecov report. Add tests for:
- Standalone mode with replicas
- Cluster mode with replicas
- Error cases (invalid mode, connection issues)
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 233-239: falkordb/falkordb.py#L233-L239
Added lines #L233 - L239 were not covered by tests
[warning] 241-243: falkordb/falkordb.py#L241-L243
Added lines #L241 - L243 were not covered by tests
falkordb/falkordb.py
Outdated
| redis_mode= self.connection.execute_command("info")['redis_mode'] | ||
| if redis_mode == "standalone": | ||
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | ||
| result = [(host,port) for host, port in replica_hostnames] | ||
| return result | ||
| elif redis_mode == "cluster": | ||
| data = self.connection.cluster_nodes() | ||
| # List comprehension to get a list of (ip, port, hostname) tuples | ||
| host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], flag['hostname']) for ip_port, flag in data.items() if 'slave' in flag["flags"]] | ||
| result = [tup for tup in host_port_list] | ||
| return result |
There was a problem hiding this comment.
Fix several critical issues in implementation
The method has multiple issues that need to be addressed:
- No error handling for invalid redis_mode
- Uses deprecated 'slave' terminology
- Potential KeyError when accessing flag['hostname']
- Inconsistent spacing around operators
Apply this diff to fix the issues:
- redis_mode= self.connection.execute_command("info")['redis_mode']
+ redis_mode = self.connection.execute_command("info", section="server").get('redis_mode')
+ if not redis_mode:
+ raise ConnectionError("Unable to determine Redis mode")
+
if redis_mode == "standalone":
- replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
+ replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)
result = [(host,port) for host, port in replica_hostnames]
return result
elif redis_mode == "cluster":
data = self.connection.cluster_nodes()
- # List comprehension to get a list of (ip, port, hostname) tuples
- host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], flag['hostname']) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
- result = [tup for tup in host_port_list]
+ result = []
+ for ip_port, flag in data.items():
+ if 'replica' in flag.get('flags', []): # Using 'replica' instead of 'slave'
+ host, port = ip_port.split(':')
+ result.append((host, int(port))) # Convert port to int for consistency
+ return result
-
+ else:
+ raise ValueError(f"Unsupported Redis mode: {redis_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.
| redis_mode= self.connection.execute_command("info")['redis_mode'] | |
| if redis_mode == "standalone": | |
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | |
| result = [(host,port) for host, port in replica_hostnames] | |
| return result | |
| elif redis_mode == "cluster": | |
| data = self.connection.cluster_nodes() | |
| # List comprehension to get a list of (ip, port, hostname) tuples | |
| host_port_list = [(ip_port.split(':')[0], ip_port.split(':')[1], flag['hostname']) for ip_port, flag in data.items() if 'slave' in flag["flags"]] | |
| result = [tup for tup in host_port_list] | |
| return result | |
| redis_mode = self.connection.execute_command("info", section="server").get('redis_mode') | |
| if not redis_mode: | |
| raise ConnectionError("Unable to determine Redis mode") | |
| if redis_mode == "standalone": | |
| replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name) | |
| result = [(host,port) for host, port in replica_hostnames] | |
| return result | |
| elif redis_mode == "cluster": | |
| data = self.connection.cluster_nodes() | |
| result = [] | |
| for ip_port, flag in data.items(): | |
| if 'replica' in flag.get('flags', []): # Using 'replica' instead of 'slave' | |
| host, port = ip_port.split(':') | |
| result.append((host, int(port))) # Convert port to int for consistency | |
| return result | |
| else: | |
| raise ValueError(f"Unsupported Redis mode: {redis_mode}") |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 233-239: falkordb/falkordb.py#L233-L239
Added lines #L233 - L239 were not covered by tests
[warning] 241-243: falkordb/falkordb.py#L241-L243
Added lines #L241 - L243 were not covered by tests
falkordb/falkordb.py
Outdated
| host_port_list = [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]] | ||
| result = [tup for tup in host_port_list] | ||
| return result |
There was a problem hiding this comment.
Should this just be:
return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]]
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
falkordb/falkordb.py (1)
231-260: Add unit tests for the new functionalityThe new
get_replica_connectionsmethod lacks test coverage. Please add comprehensive unit tests covering:
- Sentinel mode with replicas
- Cluster mode with replicas
- Error cases (connection failures, malformed data)
- Edge cases (no replicas)
Would you like me to help generate the unit test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🔇 Additional comments (1)
falkordb/falkordb.py (1)
78-78: LGTM: decode_responses parameter addition
The addition of the decode_responses parameter with a default value of True is well-implemented and correctly passed to the Redis connection.
Also applies to: 96-96
falkordb/falkordb.py
Outdated
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | ||
| return [(host, port) for host, port in replica_hostnames] |
There was a problem hiding this comment.
Update implementation to use modern Redis terminology and handle edge cases
The current implementation has several issues:
- Uses deprecated 'slave' terminology
- Assumes 'hostname' exists in cluster node flags
- 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 resultAlso applies to: 254-255
falkordb/falkordb.py
Outdated
| 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'] |
There was a problem hiding this comment.
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.
| 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)}") |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 248-254: falkordb/falkordb.py#L248-L254
Added lines #L248 - L254 were not covered by tests
[warning] 256-256: falkordb/falkordb.py#L256
Added line #L256 was not covered by tests
🪛 Ruff
falkordb/falkordb.py
256-256: redis_mode may be undefined, or defined from star imports
(F405)
🔇 Additional comments (3)
falkordb/falkordb.py (3)
78-78: LGTM: decode_responses parameter addition
The addition of the decode_responses parameter with a default value of True is well-implemented and properly passed to the Redis constructor.
Also applies to: 96-96
250-251:
Update Sentinel implementation to use modern terminology and handle edge cases
The implementation needs to use modern Redis terminology and handle potential errors.
Apply this diff:
- 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)}")Likely invalid or redundant comment.
231-259: Add test coverage for get_replica_connections method
The method lacks test coverage which could lead to undiscovered bugs.
Let's verify the current test coverage:
Would you like me to help create comprehensive test cases for this method? The tests should cover:
- Sentinel mode with successful replica discovery
- Cluster mode with successful replica discovery
- Error handling for various failure scenarios
- Invalid mode handling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 248-254: falkordb/falkordb.py#L248-L254
Added lines #L248 - L254 were not covered by tests
[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)
falkordb/falkordb.py
Outdated
| data = self.connection.cluster_nodes() | ||
| return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]] | ||
| else: | ||
| raise ValueError(f"Unsupported Redis mode: {redis_mode}") |
There was a problem hiding this comment.
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.
| 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)
falkordb/falkordb.py
Outdated
| return [(host, port) for host, port in replica_hostnames] | ||
| elif mode == "cluster": | ||
| data = self.connection.cluster_nodes() | ||
| return [(flag['hostname'], ip_port.split(':')[1]) for ip_port, flag in data.items() if 'slave' in flag["flags"]] |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
falkordb/falkordb.py (1)
78-78: Add parameter documentationThe new
decode_responsesparameter should be documented in the class docstring to maintain API documentation completeness.Add the following to the class docstring parameters section:
""" FalkorDB Class for interacting with a FalkorDB server. + Args: + decode_responses (bool, optional): Whether to decode response bytes to strings. + Defaults to True. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[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
falkordb/falkordb.py
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)
falkordb/falkordb.py
Outdated
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | ||
| return [(host, int(port)) for host, port in replica_hostnames] |
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
falkordb/falkordb.py
Outdated
| 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.
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
‼️ 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.
| try: | |
| mode = self.connection.execute_command("info")['redis_mode'] | |
| except Exception as e: | |
| raise Exception(f"Failed to get Redis mode: {e}") | |
| try: | |
| 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 | |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 248-251: falkordb/falkordb.py#L248-L251
Added lines #L248 - L251 were 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)
falkordb/falkordb.py
Outdated
| 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.
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
falkordb/falkordb.py
Outdated
| 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}") | ||
|
|
||
| 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] | ||
| 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"]] | ||
| else: | ||
| raise ValueError(f"Unsupported Redis mode: {mode}") | ||
|
|
||
|
|
||
|
|
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)
falkordb/falkordb.py
Outdated
| # 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.
if hasattr(self, 'sentinel') is true, then performing this action is a waste of time.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 249-255: falkordb/falkordb.py#L249-L255
Added lines #L249 - L255 were not covered by tests
[warning] 257-257: falkordb/falkordb.py#L257
Added line #L257 was not covered by tests
🔇 Additional comments (2)
falkordb/falkordb.py (2)
78-78: LGTM: Good default for decode_responses parameter
Setting decode_responses=True by default is a user-friendly choice as it automatically decodes Redis binary strings to Python strings.
Also applies to: 96-96
231-260: Add test coverage for the new method
The 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
Example test structure:
def test_get_replica_connections_sentinel():
# Test sentinel mode with mocked sentinel.discover_replicas
db = FalkorDB()
db.sentinel = Mock()
db.sentinel.discover_replicas.return_value = [("replica1", 6379), ("replica2", 6380)]
result = db.get_replica_connections()
assert result == [("replica1", 6379), ("replica2", 6380)]
def test_get_replica_connections_cluster():
# Test cluster mode with mocked cluster_nodes
db = FalkorDB()
db.connection.execute_command.return_value = {"redis_mode": "cluster"}
db.connection.cluster_nodes.return_value = {
"127.0.0.1:6379": {"flags": ["replica"], "hostname": "replica1"},
"127.0.0.1:6380": {"flags": ["master"]}
}
result = db.get_replica_connections()
assert result == [("replica1", 6379)]
def test_get_replica_connections_errors():
# Test connection errors and invalid modes
db = FalkorDB()
db.connection.execute_command.side_effect = redis.RedisError()
with pytest.raises(ConnectionError):
db.get_replica_connections()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-255: falkordb/falkordb.py#L249-L255
Added lines #L249 - L255 were not covered by tests
[warning] 257-257: falkordb/falkordb.py#L257
Added line #L257 was not covered by tests
falkordb/falkordb.py
Outdated
|
|
||
| mode = self.connection.execute_command("info")['redis_mode'] | ||
| 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.
🛠️ 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
falkordb/falkordb.py
Outdated
| """ | ||
| # decide if it's Sentinel or cluster | ||
|
|
||
| mode = self.connection.execute_command("info")['redis_mode'] |
There was a problem hiding this comment.
Add proper error handling
The code lacks proper error handling for Redis commands and dictionary access.
Add comprehensive error handling:
- mode = self.connection.execute_command("info")['redis_mode']
+ try:
+ 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
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):
+ continue # Skip malformed entries
+ return result
+ except redis.RedisError as e:
+ raise ConnectionError("Failed to retrieve cluster nodes") from eAlso applies to: 254-255
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-255: falkordb/falkordb.py#L249-L255
Added lines #L249 - L255 were not covered by tests
falkordb/falkordb.py
Outdated
| 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] |
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.
falkordb/falkordb.py
Outdated
| """ | ||
| # decide if it's Sentinel or cluster | ||
|
|
||
| mode = self.connection.execute_command("info")['redis_mode'] |
There was a problem hiding this comment.
Ensure that 'redis_mode' exists in the dictionary returned by the 'info' command to avoid a KeyError.
| mode = self.connection.execute_command("info")['redis_mode'] | |
| mode = self.connection.execute_command("info").get('redis_mode', None) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
falkordb/falkordb.py
Outdated
| return [(host, int(port)) for host, port in replica_hostnames] | ||
| 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.
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] |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
tests/test_get_replica_connections.py (7)
1-3: Remove unused importpatchThe
patchimport fromunittest.mockis not used in any of the test methods. All mocking is done usingMagicMockdirectly.import pytest -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock🧰 Tools
🪛 Ruff
2-2:
unittest.mock.patchimported but unusedRemove unused import:
unittest.mock.patch(F401)
8-13: Add missing decode_responses parameter to MockFalkorDBAccording to the AI summary, the
FalkorDBclass now includes adecode_responsesparameter. The mock should reflect this change for completeness.class MockFalkorDB: def __init__(self): self.connection = MagicMock() self.sentinel = None self.service_name = "mymaster" + self.decode_responses = False
4-14: Consider adding type hints for better maintainabilityAdding type hints would improve code maintainability and IDE support.
+from typing import Optional +from unittest.mock import MagicMock as TMock class TestFalkorDB: @pytest.fixture - def falkor_db(self): + def falkor_db(self) -> "MockFalkorDB": # Mocking the FalkorDB instance class MockFalkorDB: - def __init__(self): + def __init__(self) -> None: - self.connection = MagicMock() - self.sentinel = None - self.service_name = "mymaster" + self.connection: TMock = MagicMock() + self.sentinel: Optional[TMock] = None + self.service_name: str = "mymaster"
16-24: Enhance sentinel mode test with additional scenariosConsider adding tests for:
- Empty slave list
- Invalid slave address format
- Different service names
Also, consider adding type hints for better code clarity:
- def test_get_replica_connections_sentinel(self, falkor_db): + def test_get_replica_connections_sentinel(self, falkor_db: "MockFalkorDB") -> None: # Set up sentinel mode falkor_db.sentinel = MagicMock() - falkor_db.sentinel.discover_slaves.return_value = [("127.0.0.1", 6380), ("127.0.0.2", 6381)] + slave_addresses: list[tuple[str, int]] = [("127.0.0.1", 6380), ("127.0.0.2", 6381)] + falkor_db.sentinel.discover_slaves.return_value = slave_addresses result = falkor_db.get_replica_connections() - assert result == [("127.0.0.1", 6380), ("127.0.0.2", 6381)] + assert result == slave_addresses falkor_db.sentinel.discover_slaves.assert_called_once_with(service_name="mymaster")
26-38: Enhance cluster mode test coverageThe test should cover more node flag variations and edge cases:
- Mixed flags (e.g., "slave,fail")
- Different node states
- Invalid node formats
- def test_get_replica_connections_cluster(self, falkor_db): + def test_get_replica_connections_cluster(self, falkor_db: "MockFalkorDB") -> None: # Set up cluster mode falkor_db.connection.execute_command.return_value = {"redis_mode": "cluster"} falkor_db.connection.cluster_nodes.return_value = { "127.0.0.1:6379": {"hostname": "127.0.0.1", "flags": "master"}, "127.0.0.2:6380": {"hostname": "127.0.0.2", "flags": "slave"}, "127.0.0.3:6381": {"hostname": "127.0.0.3", "flags": "slave"}, + "127.0.0.4:6382": {"hostname": "127.0.0.4", "flags": "slave,fail"}, + "127.0.0.5:6383": {"hostname": "127.0.0.5", "flags": "slave,pending"}, }
40-52: Add type hints to error test methodsAdd type hints for consistency with other test methods:
- def test_get_replica_connections_unsupported_mode(self, falkor_db): + def test_get_replica_connections_unsupported_mode(self, falkor_db: "MockFalkorDB") -> None: - def test_get_replica_connections_connection_error(self, falkor_db): + def test_get_replica_connections_connection_error(self, falkor_db: "MockFalkorDB") -> None:
4-4: Add test for standalone Redis modeConsider adding a test case for standalone Redis mode to ensure complete coverage of all possible Redis configurations:
def test_get_replica_connections_standalone(self, falkor_db: "MockFalkorDB") -> None: # Set up standalone mode falkor_db.connection.execute_command.return_value = {"redis_mode": "standalone"} result = falkor_db.get_replica_connections() assert result == [] # Standalone mode should return empty list
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/test_get_replica_connections.py
2-2: unittest.mock.patch imported but unused
Remove unused import: unittest.mock.patch
(F401)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
falkordb/falkordb.py (2)
78-78: Update class docstring to document decode_responses parameterThe new parameter should be documented in the class docstring to maintain API documentation completeness.
Add the following to the class docstring parameters section:
""" + Args: + decode_responses (bool): Whether to decode response bytes to strings. Defaults to True. """Also applies to: 96-96
231-272: Add test coverage for get_replica_connectionsThe 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
Example test structure:
def test_get_replica_connections_sentinel(): # Test sentinel mode with mocked sentinel.discover_slaves db = FalkorDB() db.sentinel = Mock() db.sentinel.discover_slaves.return_value = [('host1', 6380), ('host2', 6381)] replicas = db.get_replica_connections() assert replicas == [('host1', 6380), ('host2', 6381)] def test_get_replica_connections_cluster(): # Test cluster mode with mocked cluster_nodes db = FalkorDB() db.connection.execute_command.return_value = {'redis_mode': 'cluster'} db.connection.cluster_nodes.return_value = { 'node1:6379': {'flags': ['replica'], 'hostname': 'host1'}, 'node2:6380': {'flags': ['replica'], 'hostname': 'host2'} } replicas = db.get_replica_connections() assert replicas == [('host1', 6379), ('host2', 6380)]🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-256: falkordb/falkordb.py#L249-L256
Added lines #L249 - L256 were not covered by tests
[warning] 258-266: falkordb/falkordb.py#L258-L266
Added lines #L258 - L266 were not covered by tests
[warning] 269-269: falkordb/falkordb.py#L269
Added line #L269 was not covered by tests🪛 Ruff
255-255: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
256-256: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
265-265: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
266-266: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (1)
falkordb/falkordb.py(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 249-256: falkordb/falkordb.py#L249-L256
Added lines #L249 - L256 were not covered by tests
[warning] 258-266: falkordb/falkordb.py#L258-L266
Added lines #L258 - L266 were not covered by tests
[warning] 269-269: falkordb/falkordb.py#L269
Added line #L269 was not covered by tests
🪛 Ruff
falkordb/falkordb.py
255-255: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
256-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
265-265: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
266-266: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
falkordb/falkordb.py
Outdated
| if hasattr(self, 'sentinel') and self.sentinel is not None: | ||
| try: | ||
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | ||
| if len(replica_hostnames) == 0: | ||
| raise ConnectionError("Unable to get replica hostname.") | ||
| return [(host, int(port)) for host, port in replica_hostnames] | ||
| except redis.RedisError as e: | ||
| raise ConnectionError("Failed to get replica hostnames, no hostnames found.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in Sentinel mode
The exception handling needs improvement:
- Unused exception variable
- Missing exception chaining
Apply this diff:
try:
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
if len(replica_hostnames) == 0:
raise ConnectionError("Unable to get replica hostname.")
return [(host, int(port)) for host, port in replica_hostnames]
- except redis.RedisError as e:
- raise ConnectionError("Failed to get replica hostnames, no hostnames found.")
+ except redis.RedisError as err:
+ raise ConnectionError("Failed to get replica hostnames, no hostnames found.") from err📝 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.
| if hasattr(self, 'sentinel') and self.sentinel is not None: | |
| try: | |
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | |
| if len(replica_hostnames) == 0: | |
| raise ConnectionError("Unable to get replica hostname.") | |
| return [(host, int(port)) for host, port in replica_hostnames] | |
| except redis.RedisError as e: | |
| raise ConnectionError("Failed to get replica hostnames, no hostnames found.") | |
| if hasattr(self, 'sentinel') and self.sentinel is not None: | |
| try: | |
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | |
| if len(replica_hostnames) == 0: | |
| raise ConnectionError("Unable to get replica hostname.") | |
| return [(host, int(port)) for host, port in replica_hostnames] | |
| except redis.RedisError as err: | |
| raise ConnectionError("Failed to get replica hostnames, no hostnames found.") from err |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-256: falkordb/falkordb.py#L249-L256
Added lines #L249 - L256 were not covered by tests
🪛 Ruff
255-255: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
256-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
falkordb/falkordb.py
Outdated
| mode = self.connection.execute_command("info").get('redis_mode', None) | ||
| if mode == "cluster": | ||
| try: | ||
| data = self.connection.cluster_nodes() | ||
| if not data: | ||
| raise ConnectionError("Unable to get 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'] and len(ip_port.split(':')) > 1] | ||
| except redis.RedisError as e: | ||
| raise ConnectionError("Failed to get replica hostnames") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cluster mode implementation
Several issues need to be addressed:
- Unsafe dictionary access
- Deprecated terminology
- Exception handling
Apply this diff:
mode = self.connection.execute_command("info").get('redis_mode', None)
if mode == "cluster":
try:
data = self.connection.cluster_nodes()
if not data:
raise ConnectionError("Unable to get 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'] and len(ip_port.split(':')) > 1]
+ 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):
+ continue # Skip malformed entries
+ return result
- except redis.RedisError as e:
- raise ConnectionError("Failed to get replica hostnames")
+ except redis.RedisError as err:
+ raise ConnectionError("Failed to get replica hostnames") from err📝 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.
| mode = self.connection.execute_command("info").get('redis_mode', None) | |
| if mode == "cluster": | |
| try: | |
| data = self.connection.cluster_nodes() | |
| if not data: | |
| raise ConnectionError("Unable to get 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'] and len(ip_port.split(':')) > 1] | |
| except redis.RedisError as e: | |
| raise ConnectionError("Failed to get replica hostnames") | |
| mode = self.connection.execute_command("info").get('redis_mode', None) | |
| if mode == "cluster": | |
| try: | |
| data = self.connection.cluster_nodes() | |
| if not data: | |
| raise ConnectionError("Unable to get 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): | |
| continue # Skip malformed entries | |
| return result | |
| except redis.RedisError as err: | |
| raise ConnectionError("Failed to get replica hostnames") from err |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-266: falkordb/falkordb.py#L258-L266
Added lines #L258 - L266 were not covered by tests
🪛 Ruff
265-265: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
266-266: 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.
Actionable comments posted: 8
♻️ Duplicate comments (4)
.github/workflows/test.yml (2)
20-23: 🛠️ Refactor suggestionAdd health check wait before running tests.
The workflow should wait for the Redis standalone service to be healthy before proceeding with tests.
- name: Run docker-compose for redis standalone instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/standalone-compose.yml + - name: Wait for Redis standalone to be ready + run: | + timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done'
30-33: 🛠️ Refactor suggestionAdd health check for cluster setup.
Add health check for cluster setup as well.
- name: Run docker-compose for redis cluster instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/cluster-compose.yml + - name: Wait for cluster to be ready + run: | + timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'tests/test_get_replica_connections.py (2)
6-7:⚠️ Potential issueSecurity: Sanitize inputs in subprocess calls.
The container deletion function has potential security vulnerabilities.
def delete_container(container_name): - subprocess.run(["docker","rm",container_name,'-f'],check=True,encoding='utf-8') + if not isinstance(container_name, str) or not container_name.isalnum(): + raise ValueError("Invalid container name") + try: + subprocess.run(["docker", "rm", container_name, '-f'], + check=True, encoding='utf-8', shell=False) + except subprocess.CalledProcessError as e: + raise RuntimeError(f"Failed to delete container: {e}")
70-78:⚠️ Potential issueFix inconsistent compose file paths.
The paths to compose files are inconsistent and potentially incorrect.
- reapply_compose('/home/runner/work/falkordb-py/falkordb-py/cluster-compose') + reapply_compose('./docker/cluster-compose.yml')
🧹 Nitpick comments (5)
docker/standalone-compose.yml (1)
1-7: Add healthcheck configuration.Adding a healthcheck will ensure the service is fully operational before tests begin, reducing flaky tests.
image: falkordb/falkordb:edge ports: - 6379:6379 + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 5s + timeout: 3s + retries: 3tests/test_get_replica_connections.py (1)
58-58: Remove unused variable assignment.The
resultvariable is assigned but never used.- result = c.get_replica_connections() + c.get_replica_connections()🧰 Tools
🪛 Ruff (0.8.2)
58-58: Local variable
resultis assigned to but never usedRemove assignment to unused variable
result(F841)
docker/sentinel-compose.yml (1)
78-83: Use environment variables for configurationConsider using environment variables for configuration values like ports and module paths to make the configuration more flexible and maintainable.
Example:
+ environment: + - REDIS_PORT=6380 + - FALKORDB_MODULE_PATH=/FalkorDB/bin/src/falkordb.so command: - bash - -c - | touch /data/node.conf - echo "port 6380" >> /data/node.conf + echo "port $REDIS_PORT" >> /data/node.conf echo "replica-announce-ip redis-server-1" >> /data/node.conf - echo "replica-announce-port 6380" >> /data/node.conf + echo "replica-announce-port $REDIS_PORT" >> /data/node.conf - echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf + echo "loadmodule $FALKORDB_MODULE_PATH" >> /data/node.confAlso applies to: 96-102
🧰 Tools
🪛 yamllint (1.35.1)
[error] 79-79: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml (2)
22-31: Use environment variables for configurationConsider using environment variables for configuration values to improve maintainability and reduce duplication.
Example for node configuration:
+ environment: + - REDIS_PORT=5000 + - NODE_NAME=node0 + - FALKORDB_MODULE_PATH=/FalkorDB/bin/src/falkordb.so + - CLUSTER_PORT=16379 + - CLUSTER_TIMEOUT=5000 command: - bash - -c - | touch /data/node.conf - echo "port 5000" >> /data/node.conf + echo "port $REDIS_PORT" >> /data/node.conf - echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf + echo "loadmodule $FALKORDB_MODULE_PATH" >> /data/node.conf echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf - echo "cluster-port 16379" >> /data/node.conf + echo "cluster-port $CLUSTER_PORT" >> /data/node.conf echo "cluster-enabled yes" >> /data/node.conf echo "cluster-config-file /data/nodes.conf" >> /data/node.conf - echo "cluster-node-timeout 5000" >> /data/node.conf + echo "cluster-node-timeout $CLUSTER_TIMEOUT" >> /data/node.conf - echo "cluster-announce-hostname node0" >> /data/node.conf + echo "cluster-announce-hostname $NODE_NAME" >> /data/node.confAlso applies to: 44-53, 66-75, 88-97, 110-119, 133-142
145-146: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
networks: redis-cluster: +🧰 Tools
🪛 yamllint (1.35.1)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(4 hunks)docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)docker/standalone-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
[warning] 41-41: wrong indentation: expected 6 but found 4
(indentation)
[warning] 63-63: wrong indentation: expected 6 but found 4
(indentation)
[warning] 85-85: wrong indentation: expected 6 but found 4
(indentation)
[warning] 107-107: wrong indentation: expected 6 but found 4
(indentation)
[warning] 130-130: wrong indentation: expected 6 but found 4
(indentation)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[warning] 12-12: wrong indentation: expected 6 but found 4
(indentation)
[warning] 33-33: wrong indentation: expected 6 but found 4
(indentation)
[warning] 54-54: wrong indentation: expected 6 but found 4
(indentation)
[warning] 75-75: wrong indentation: expected 6 but found 4
(indentation)
[error] 79-79: trailing spaces
(trailing-spaces)
[warning] 93-93: wrong indentation: expected 6 but found 4
(indentation)
[error] 97-97: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
58-58: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (3)
.github/workflows/test.yml (1)
25-28: 🛠️ Refactor suggestion
Add health check for sentinel setup.
Similar to standalone setup, add health check for sentinel setup.
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done'Likely invalid or redundant comment.
tests/test_get_replica_connections.py (2)
12-33: Improve client initialization functions.
The client initialization functions could be improved for better maintainability.
from typing import Callable
from dataclasses import dataclass
@dataclass
class ClientConfig:
host: str = 'localhost'
ports: dict[str, int] = field(default_factory=lambda: {
'cluster': 5000,
'standalone': 6379,
'sentinel': 26379
})
def create_client(mode: str) -> Callable[[], FalkorDB]:
"""Create a FalkorDB client factory function for the specified mode."""
config = ClientConfig()
def client_factory() -> FalkorDB:
return FalkorDB(
host=config.host,
port=config.ports[mode]
)
return client_factory
cluster_client = create_client('cluster')
standalone_client = create_client('standalone')
sentinel_client = create_client('sentinel')9-10:
Security: Fix shell injection vulnerability.
Using shell=True with f-strings can lead to command injection.
def reapply_compose(path):
- subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d",check=True,shell=True,encoding='utf-8')
+ if not isinstance(path, str) or '..' in path:
+ raise ValueError("Invalid path")
+ try:
+ subprocess.run(["docker-compose", "-f", path, "down"],
+ check=True, encoding='utf-8', shell=False)
+ subprocess.run(["docker-compose", "-f", path, "up", "-d"],
+ check=True, encoding='utf-8', shell=False)
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to reapply compose: {e}")Likely invalid or redundant comment.
docker/standalone-compose.yml
Outdated
| redis-standalone: | ||
| container_name: redis-standalone | ||
| hostname: redis-standalone | ||
| image: falkordb/falkordb:edge |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a stable version tag instead of edge.
Using the edge tag can lead to inconsistent test results as the image could change unexpectedly. Consider using a specific version tag for better stability and reproducibility.
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latestCommittable suggestion skipped: line range outside the PR's diff.
| def test_get_replica_connections_cluster_no_replicas(): | ||
| # Assume this cluster has no replicas configured | ||
| delete_replicas(cluster_client(),'cluster') | ||
| time.sleep(15) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded sleep times with proper health checks.
Using fixed sleep times can lead to flaky tests. Consider using health checks instead.
def wait_for_health(client, timeout=30):
"""Wait for the service to be healthy with exponential backoff."""
start = time.time()
delay = 1
while time.time() - start < timeout:
try:
client.ping()
return True
except Exception:
time.sleep(delay)
delay = min(delay * 2, 5)
raise TimeoutError("Service did not become healthy")Also applies to: 83-83
docker/sentinel-compose.yml
Outdated
| @@ -0,0 +1,105 @@ | |||
| services: | |||
| sentinel-1: | |||
| image: muhammadqadora/falkor-redis | |||
There was a problem hiding this comment.
Use standard FalkorDB/FalkorDB Docker image
As requested in the PR comments, please use the standard FalkorDB/FalkorDB Docker image instead of muhammadqadora/falkor-redis. This ensures consistency and reliability across deployments.
- image: muhammadqadora/falkor-redis
+ image: falkordb/falkordb:latestAlso applies to: 24-24, 45-45, 67-67, 85-85
docker/sentinel-compose.yml
Outdated
| redis-server-1: | ||
| image: muhammadqadora/falkor-redis | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| command: | ||
| - bash | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
Add health check and restart policy for Redis master
The Redis master server lacks health checks and a restart policy, which are crucial for maintaining high availability.
redis-server-1:
image: muhammadqadora/falkor-redis
hostname: redis-server-1
container_name: redis-server-1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6380", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
ports:
- 6380:6380
networks:
- redis-sentinel📝 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.
| redis-server-1: | |
| image: muhammadqadora/falkor-redis | |
| hostname: redis-server-1 | |
| container_name: redis-server-1 | |
| ports: | |
| - 6380:6380 | |
| networks: | |
| - redis-sentinel | |
| command: | |
| - bash | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6380" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | |
| echo "replica-announce-port 6380" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| redis-server /data/node.conf | |
| redis-server-1: | |
| image: muhammadqadora/falkor-redis | |
| hostname: redis-server-1 | |
| container_name: redis-server-1 | |
| restart: always | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "6380", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 3 | |
| ports: | |
| - 6380:6380 | |
| networks: | |
| - redis-sentinel | |
| command: | |
| - bash | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6380" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | |
| echo "replica-announce-port 6380" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| redis-server /data/node.conf |
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 75-75: wrong indentation: expected 6 but found 4
(indentation)
[error] 79-79: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-2: | ||
| image: muhammadqadora/falkor-redis | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| command: | ||
| - bash | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6381" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | ||
| echo "replica-announce-port 6381" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "replicaof redis-server-1 6380" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
Add health check and restart policy for Redis replica
Similar to the master, the replica server should have health checks and a restart policy.
redis-server-2:
image: muhammadqadora/falkor-redis
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
ports:
- 6381:6381
networks:
- redis-sentinel📝 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.
| redis-server-2: | |
| image: muhammadqadora/falkor-redis | |
| hostname: redis-server-2 | |
| container_name: redis-server-2 | |
| ports: | |
| - 6381:6381 | |
| networks: | |
| - redis-sentinel | |
| command: | |
| - bash | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6381" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | |
| echo "replica-announce-port 6381" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| echo "replicaof redis-server-1 6380" >> /data/node.conf | |
| redis-server /data/node.conf | |
| redis-server-2: | |
| image: muhammadqadora/falkor-redis | |
| hostname: redis-server-2 | |
| container_name: redis-server-2 | |
| restart: always | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "6381", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 3 | |
| ports: | |
| - 6381:6381 | |
| networks: | |
| - redis-sentinel | |
| command: | |
| - bash | |
| - -c | |
| - | | |
| touch /data/node.conf | |
| echo "port 6381" >> /data/node.conf | |
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | |
| echo "replica-announce-port 6381" >> /data/node.conf | |
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | |
| echo "replicaof redis-server-1 6380" >> /data/node.conf | |
| redis-server /data/node.conf |
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 93-93: wrong indentation: expected 6 but found 4
(indentation)
[error] 97-97: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
Outdated
| @@ -0,0 +1,146 @@ | |||
| services: | |||
| node0: | |||
| image: muhammadqadora/falkor-redis | |||
There was a problem hiding this comment.
Use standard FalkorDB/FalkorDB Docker image
As requested in the PR comments, please use the standard FalkorDB/FalkorDB Docker image instead of muhammadqadora/falkor-redis.
- image: muhammadqadora/falkor-redis
+ image: falkordb/falkordb:latestAlso applies to: 33-33, 55-55, 77-77, 99-99, 122-122
docker/cluster-compose.yml
Outdated
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 2s |
There was a problem hiding this comment.
Fix health check logic and add init container for cluster creation
The current health check has several issues:
- The cluster creation command is not appropriate for health checking
- The
|| echo failed && exit 1logic is incorrect (will always exit) - Health checks should verify cluster state, not create it
Consider moving cluster creation to an init container and implementing proper health checks.
- healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 2s
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "5000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ cluster-init:
+ condition: service_completed_successfullyAdd a new service for cluster initialization:
cluster-init:
image: falkordb/falkordb:latest
command: >
/bin/bash -c "
echo 'Waiting for nodes to be ready...' &&
sleep 10 &&
redis-cli --cluster create node0:5000 node1:6000 node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes
"
networks:
- redis-cluster
depends_on:
node0:
condition: service_healthy
node1:
condition: service_healthy
node2:
condition: service_healthy
node3:
condition: service_healthy
node4:
condition: service_healthy
node5:
condition: service_healthy🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
docker/cluster-compose.yml
Outdated
| node0: | ||
| image: muhammadqadora/falkor-redis | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 2s | ||
| command: | ||
| - bash | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 5000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node0" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
Add restart policies and health checks for all nodes
Each node in the cluster should have a restart policy and health check to ensure proper operation.
Example for node0 (apply similar changes to all nodes):
node0:
image: muhammadqadora/falkor-redis
container_name: node0
hostname: node0
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "5000", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
ports:
- 5000:5000
networks:
- redis-clusterCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/test_get_replica_connections.py (3)
15-35: Improve client initialization functionsThe client initialization functions could be improved:
- Code duplication in initialization pattern
- Missing docstrings and type hints
- Hardcoded connection parameters
Consider this improved implementation:
from typing import Callable from dataclasses import dataclass, field @dataclass class ClientConfig: host: str = 'localhost' ports: dict[str, int] = field(default_factory=lambda: { 'cluster': 5000, 'standalone': 6379, 'sentinel': 26379 }) def create_client(mode: str) -> Callable[[], FalkorDB]: """Create a FalkorDB client factory function for the specified mode. Args: mode: One of 'cluster', 'standalone', or 'sentinel' Returns: A factory function that creates a configured FalkorDB client """ config = ClientConfig() def client_factory() -> FalkorDB: return FalkorDB( host=config.host, port=config.ports[mode] ) return client_factory cluster_client = create_client('cluster') standalone_client = create_client('standalone') sentinel_client = create_client('sentinel')
48-54: Make test more robust and maintainableThe test has several issues:
- Hardcoded node configurations make it brittle
- Missing setup/teardown to ensure consistent cluster state
- No validation of connection properties beyond port numbers
Consider using a fixture to manage cluster state and parameterizing the test:
@pytest.fixture def cluster_nodes(): return [ ('node3', 8000), ('node4', 9000), ('node5', 10000) ] def test_get_replica_connections_cluster(cluster_nodes): c = cluster_client() result = c.get_replica_connections() for node in cluster_nodes: assert node in result # Validate additional connection properties
58-68: Improve test assertions and remove unused variableMinor improvements needed:
- Remove unused
resultvariable intest_get_replica_connections_standalone- Add more specific error message assertions
Apply these changes:
def test_get_replica_connections_standalone(): c = standalone_client() with pytest.raises(ValueError, match="Unsupported Redis mode"): - result = c.get_replica_connections() + c.get_replica_connections() def test_get_replica_connections_sentinel(): c = sentinel_client() result = c.get_replica_connections() - assert result == [('redis-server-2', 6381)] + assert result == [('redis-server-2', 6381)], "Unexpected sentinel replica configuration"🧰 Tools
🪛 Ruff (0.8.2)
61-61: Local variable
resultis assigned to but never usedRemove assignment to unused variable
result(F841)
docker/cluster-compose.yml (1)
133-134: Improve network configurationMinor improvements needed:
- Add newline at end of file
- Consider explicit network configuration
Add explicit network configuration:
networks: redis-cluster: + name: redis-cluster + driver: bridge + ipam: + driver: default + config: + - subnet: 172.20.0.0/16 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 134-134: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/cluster-compose.yml
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[error] 134-134: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 73-73: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 90-90: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
61-61: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (2)
tests/test_get_replica_connections.py (1)
8-13:
Improve security in subprocess calls
The utility functions have potential security vulnerabilities:
- Using
shell=Truewith f-strings can lead to command injection - No input validation for
container_nameandpathparameters - Missing error handling for subprocess failures
Apply this fix:
def delete_container(container_name):
+ if not isinstance(container_name, str) or not container_name.isalnum():
+ raise ValueError("Invalid container name")
+ try:
- subprocess.run(["docker","rm",container_name,'-f'],check=True,encoding='utf-8')
+ subprocess.run(["docker", "rm", container_name, '-f'],
+ check=True, encoding='utf-8', shell=False)
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to delete container: {e}")
def reapply_compose(path):
+ if not isinstance(path, str) or '..' in path:
+ raise ValueError("Invalid path")
+ try:
- subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d",check=True,shell=True,encoding='utf-8')
+ subprocess.run(["docker-compose", "-f", path, "down"],
+ check=True, encoding='utf-8', shell=False)
+ subprocess.run(["docker-compose", "-f", path, "up", "-d"],
+ check=True, encoding='utf-8', shell=False)
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to reapply compose: {e}")Likely invalid or redundant comment.
docker/cluster-compose.yml (1)
30-130: 🛠️ Refactor suggestion
Add health checks and restart policies for all nodes
Issues in remaining node configurations:
- Missing health checks
- Missing restart policies
- Inconsistent configuration alignment
Apply these improvements to all nodes (node1 through node5):
node1:
image: falkordb/falkordb:edge
container_name: node1
hostname: node1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6000", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
ports:
- 6000:6000
networks:
- redis-clusterAlso, fix the alignment in configuration files for better readability:
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf
+ echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
+ echo "cluster-preferred-endpoint-type hostname" >> /data/node.confLikely invalid or redundant comment.
| def test_get_replica_connections_cluster_no_replicas(): | ||
| # Assume this cluster has no replicas configured | ||
| delete_replicas(cluster_client(),'cluster') | ||
| time.sleep(15) | ||
| c = cluster_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get cluster nodes"): | ||
| c.get_replica_connections() | ||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/cluster-compose') | ||
|
|
||
|
|
||
| def test_get_replica_connections_sentinel_no_replicas(): | ||
| # Assume this Sentinel setup has no replicas | ||
| delete_replicas(sentinel_client(),'sentinel') | ||
| time.sleep(40) | ||
| c = sentinel_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get replica hostname."): | ||
| c.get_replica_connections() | ||
|
|
||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/sentinel-compose') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and consistency
The no-replicas tests have several issues:
- Hardcoded sleep values (15s and 40s) which could cause flaky tests
- Inconsistent compose file paths
- No cleanup in case of test failure
Consider these improvements:
import os
from contextlib import contextmanager
COMPOSE_PATHS = {
'cluster': './docker/cluster-compose',
'sentinel': './docker/sentinel-compose'
}
STARTUP_DELAYS = {
'cluster': 15,
'sentinel': 40
}
@contextmanager
def temporary_no_replicas(mode: str):
"""Context manager to temporarily remove replicas and restore them after test."""
client = globals()[f"{mode}_client"]()
try:
delete_replicas(client, mode)
time.sleep(STARTUP_DELAYS[mode])
yield
finally:
reapply_compose(COMPOSE_PATHS[mode])
time.sleep(STARTUP_DELAYS[mode])
def test_get_replica_connections_cluster_no_replicas():
with temporary_no_replicas('cluster'):
c = cluster_client()
with pytest.raises(ConnectionError, match="Unable to get cluster nodes"):
c.get_replica_connections()
def test_get_replica_connections_sentinel_no_replicas():
with temporary_no_replicas('sentinel'):
c = sentinel_client()
with pytest.raises(ConnectionError, match="Unable to get replica hostname."):
c.get_replica_connections()
docker/sentinel-compose.yml
Outdated
| sentinel-1: | ||
| image: falkordb/falkordb:edge | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-2: | ||
| image: falkordb/falkordb:edge | ||
| container_name: sentinel-2 | ||
| hostname: sentinel-2 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26380:26380 | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-2" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26380" >> /data/sentinel.conf | ||
| echo "port 26380" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-3: | ||
| image: falkordb/falkordb:edge | ||
| container_name: sentinel-3 | ||
| hostname: sentinel-3 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26381:26381 | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-3" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26381" >> /data/sentinel.conf | ||
| echo "port 26381" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks and improve reliability for sentinel services
The sentinel services need improvements:
- Missing health checks
- Inconsistent restart policies
- No dependencies between services
Add health checks and dependencies:
sentinel-1:
image: falkordb/falkordb:edge
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
networks:
- redis-sentinel
ports:
- 26379:26379Apply similar changes to sentinel-2 and sentinel-3. Also, add dependencies:
depends_on:
redis-server-1:
condition: service_healthy
redis-server-2:
condition: service_healthy🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-1: | ||
| image: falkordb/falkordb:edge | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis master server configuration
The Redis master server configuration needs improvements:
- Missing health check
- Missing restart policy
- No volume for data persistence
Add the following configurations:
redis-server-1:
image: falkordb/falkordb:edge
hostname: redis-server-1
container_name: redis-server-1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6380", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - redis_data:/data
ports:
- 6380:6380
networks:
- redis-sentinelAdd at the end of the file:
volumes:
redis_data:🧰 Tools
🪛 yamllint (1.35.1)
[error] 73-73: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-2: | ||
| image: falkordb/falkordb:edge | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/node.conf | ||
| echo "port 6381" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | ||
| echo "replica-announce-port 6381" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "replicaof redis-server-1 6380" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis replica server configuration
The Redis replica server has similar issues:
- Missing health check
- Missing restart policy
- No replication delay configuration
Add the following configurations:
redis-server-2:
image: falkordb/falkordb:edge
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - redis_replica_data:/data
ports:
- 6381:6381
networks:
- redis-sentinel
+ depends_on:
+ redis-server-1:
+ condition: service_healthyAdd to volumes section:
redis_replica_data:🧰 Tools
🪛 yamllint (1.35.1)
[error] 90-90: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
Outdated
| node0: | ||
| image: falkordb/falkordb:edge | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 2s | ||
| entrypoint: ["/bin/bash", "-c"] | ||
| command: | | ||
| touch /data/node.conf | ||
| echo "port 5000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node0" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
Fix cluster initialization and health check logic
Critical issues in node0 configuration:
- Cluster creation in healthcheck is incorrect
- Missing restart policy
- No proper health verification
Implement proper initialization and health checks:
node0:
image: falkordb/falkordb:edge
container_name: node0
hostname: node0
+ restart: always
ports:
- 5000:5000
networks:
- redis-cluster
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 2s
+ test: ["CMD", "redis-cli", "-p", "5000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3Add a new service for cluster initialization:
cluster-init:
image: falkordb/falkordb:edge
command: >
/bin/bash -c "
echo 'Waiting for nodes to be ready...' &&
sleep 10 &&
redis-cli --cluster create node0:5000 node1:6000 node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes
"
networks:
- redis-cluster
depends_on:
node0:
condition: service_healthy
node1:
condition: service_healthy
node2:
condition: service_healthy
node3:
condition: service_healthy
node4:
condition: service_healthy
node5:
condition: service_healthy🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
docker/sentinel-compose.yml (1)
13-13: Remove trailing spacesThere are trailing spaces in several lines that should be removed for consistency.
Remove trailing spaces from lines 13, 45, 81, 86, and 100.
Also applies to: 45-45, 81-81, 86-86, 100-100
🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml (2)
3-3: Use stable image tag instead of edgeThe
edgetag might be unstable for production use. Consider using a specific version tag orlatestfor better stability.- image: falkordb/falkordb:edge + image: falkordb/falkordb:latest
145-146: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
networks: redis-cluster: +🧰 Tools
🪛 yamllint (1.35.1)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/cluster-compose.yml
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 81-81: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 100-100: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
docker/sentinel-compose.yml (1)
107-108: Network configuration looks good
The network configuration is minimal but sufficient for the Redis Sentinel setup.
docker/cluster-compose.yml (2)
2-31: 🛠️ Refactor suggestion
Add restart policy and fix node configuration
The node configuration is missing important reliability settings.
node0:
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: unless-stopped
ports:
- 5000:5000
networks:
- redis-clusterLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
10-17:
Fix health check implementation
The current health check has several issues:
- Using cluster creation command for health check is incorrect
- The
|| echo failed && exit 1logic is flawed - Missing proper health verification
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 2s
+ test: ["CMD", "redis-cli", "-p", "5000", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
docker/sentinel-compose.yml
Outdated
| sentinel-1: | ||
| image: falkordb/falkordb:edge | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks and improve reliability for sentinel services
The sentinel services need several improvements for production readiness:
- Missing health checks for monitoring service health
- No dependencies defined on Redis master
- Using unstable edge tag
- Runtime configuration instead of mounted configs
Apply these changes to all sentinel services:
sentinel-1:
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latest
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - ./sentinel1.conf:/data/sentinel.conf:ro
networks:
- redis-sentinel
ports:
- 26379:26379Consider moving the sentinel configuration to external files for better maintainability.
Also applies to: 24-44, 46-66
🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-1: | ||
| image: falkordb/falkordb:edge | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis master server configuration
The Redis master configuration needs several improvements for production readiness:
- Missing health check
- No restart policy
- No volume for data persistence
- Using unstable edge tag
Apply these changes:
redis-server-1:
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latest
hostname: redis-server-1
container_name: redis-server-1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6380", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - redis_master_data:/data
+ - ./master.conf:/data/node.conf:ro
ports:
- 6380:6380
networks:
- redis-sentinelAdd at the end of the file:
volumes:
redis_master_data:🧰 Tools
🪛 yamllint (1.35.1)
[error] 81-81: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-2: | ||
| image: falkordb/falkordb:edge | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6381" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | ||
| echo "replica-announce-port 6381" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "replicaof redis-server-1 6380" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis replica server configuration
The Redis replica configuration needs several improvements for production readiness:
- Missing health check
- No restart policy
- No volume for data persistence
- Using unstable edge tag
- No explicit dependency on master
Apply these changes:
redis-server-2:
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latest
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - redis_replica_data:/data
+ - ./replica.conf:/data/node.conf:ro
ports:
- 6381:6381
networks:
- redis-sentinelAdd to volumes section:
redis_replica_data:🧰 Tools
🪛 yamllint (1.35.1)
[error] 100-100: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
Outdated
| - | | ||
| touch /data/node.conf | ||
| echo "port 6000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node1" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node2: | ||
| image: falkordb/falkordb:edge | ||
| container_name: node2 | ||
| hostname: node2 | ||
| ports: | ||
| - 7000:7000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 7000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node2" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node3: | ||
| image: falkordb/falkordb:edge | ||
| container_name: node3 | ||
| hostname: node3 | ||
| ports: | ||
| - 8000:8000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 8000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node3" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node4: | ||
| image: falkordb/falkordb:edge | ||
| container_name: node4 | ||
| hostname: node4 | ||
| ports: | ||
| - 9000:9000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 9000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node4" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
|
|
||
| node5: | ||
| image: falkordb/falkordb:edge | ||
| container_name: node5 | ||
| hostname: node5 | ||
| ports: | ||
| - 10000:10000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 10000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node5" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent configuration across all nodes
The remaining nodes (node1 through node5) need the same improvements as node0:
- Use stable image tag
- Add health checks
- Add restart policies
Example for node1 (apply similar changes to all nodes):
node1:
- image: falkordb/falkordb:edge
+ image: falkordb/falkordb:latest
container_name: node1
hostname: node1
+ restart: unless-stopped
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6000", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
ports:
- 6000:6000Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)docker/standalone-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/standalone-compose.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/cluster-compose.yml
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 81-81: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 100-100: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
61-61: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (2)
tests/test_get_replica_connections.py (2)
12-13:
Fix critical security vulnerability in Docker Compose execution.
The function has several critical issues:
- Using
shell=Truewith f-strings can lead to command injection - No input validation for
pathparameter - Missing error handling for subprocess failures
Apply this diff to fix the security issues and improve error handling:
def reapply_compose(path):
+ if not isinstance(path, str) or '..' in path:
+ raise ValueError("Invalid path")
+ try:
- subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d",check=True,shell=True,encoding='utf-8')
+ subprocess.run(["docker-compose", "-f", path, "down"], check=True, encoding='utf-8', shell=False)
+ subprocess.run(["docker-compose", "-f", path, "up", "-d"], check=True, encoding='utf-8', shell=False)
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to reapply compose: {e}")Likely invalid or redundant comment.
8-9:
Improve security and error handling for container deletion.
The function needs input validation and proper error handling:
- No validation of container_name parameter
- Missing error handling for subprocess failures
Apply this diff to improve security and error handling:
def delete_container(container_name):
+ if not isinstance(container_name, str) or not container_name.isalnum():
+ raise ValueError("Invalid container name")
+ try:
subprocess.run(["docker","rm",container_name,'-f'],check=True,encoding='utf-8')
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Failed to delete container: {e}")Likely invalid or redundant comment.
| def cluster_client(): | ||
| client = FalkorDB( | ||
| host='localhost', | ||
| port=5000 | ||
| ) | ||
| return client | ||
|
|
||
|
|
||
| def standalone_client(): | ||
| client = FalkorDB( | ||
| host='localhost', | ||
| port=6379 | ||
| ) | ||
| return client | ||
|
|
||
| def sentinel_client(): | ||
| client = FalkorDB( | ||
| host='localhost', | ||
| port=26379 | ||
| ) | ||
| return client |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve client initialization functions.
The client initialization functions have several issues:
- Code duplication in initialization pattern
- Missing docstrings and type hints
- Hardcoded connection parameters make tests less maintainable
Consider this improved implementation:
from typing import Callable
from dataclasses import dataclass
@dataclass
class ClientConfig:
host: str = 'localhost'
ports: dict[str, int] = field(default_factory=lambda: {
'cluster': 5000,
'standalone': 6379,
'sentinel': 26379
})
def create_client(mode: str) -> Callable[[], FalkorDB]:
"""Create a FalkorDB client factory function for the specified mode.
Args:
mode: One of 'cluster', 'standalone', or 'sentinel'
Returns:
A factory function that creates a configured FalkorDB client
"""
config = ClientConfig()
def client_factory() -> FalkorDB:
return FalkorDB(
host=config.host,
port=config.ports[mode]
)
return client_factory
cluster_client = create_client('cluster')
standalone_client = create_client('standalone')
sentinel_client = create_client('sentinel')| def delete_replicas(client,mode): | ||
| result = client.get_replica_connections() | ||
| if mode == 'cluster': | ||
| for i in result: | ||
| name = i[0] | ||
| delete_container(name) | ||
| elif mode == 'sentinel': | ||
| name = result[0][0] | ||
| delete_container(name) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and function structure.
The function has several issues:
- No error handling for get_replica_connections
- No logging of errors
- Complex nested conditionals
Consider this improved implementation:
import logging
from typing import Literal
logger = logging.getLogger(__name__)
def delete_replicas(client: FalkorDB, mode: Literal['cluster', 'sentinel']) -> None:
"""Delete replica containers for the specified mode.
Args:
client: FalkorDB client instance
mode: Either 'cluster' or 'sentinel'
"""
try:
replicas = client.get_replica_connections()
except Exception:
logger.exception("Failed to get replica connections")
return
def delete_replica(name: str) -> None:
try:
delete_container(name)
except Exception:
logger.exception(f"Failed to delete container {name}")
if mode == 'cluster':
for name, _ in replicas:
delete_replica(name)
elif mode == 'sentinel':
if replicas:
delete_replica(replicas[0][0])| def test_get_replica_connections_cluster_no_replicas(): | ||
| # Assume this cluster has no replicas configured | ||
| delete_replicas(cluster_client(),'cluster') | ||
| time.sleep(15) | ||
| c = cluster_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get cluster nodes"): | ||
| c.get_replica_connections() | ||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/cluster-compose.yml') | ||
|
|
||
|
|
||
| def test_get_replica_connections_sentinel_no_replicas(): | ||
| # Assume this Sentinel setup has no replicas | ||
| delete_replicas(sentinel_client(),'sentinel') | ||
| time.sleep(40) | ||
| c = sentinel_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get replica hostname."): | ||
| c.get_replica_connections() | ||
|
|
||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/sentinel-compose.yml') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and consistency.
The no-replicas tests have several issues:
- Hardcoded sleep values (15s and 40s) which could cause flaky tests
- Inconsistent compose file paths
- No cleanup in case of test failure
Consider these improvements:
import os
from contextlib import contextmanager
COMPOSE_PATHS = {
'cluster': './docker/cluster-compose.yml',
'sentinel': './docker/sentinel-compose.yml'
}
def wait_for_health(client, timeout=30):
"""Wait for the service to be healthy with exponential backoff."""
start = time.time()
delay = 1
while time.time() - start < timeout:
try:
client.ping()
return True
except Exception:
time.sleep(delay)
delay = min(delay * 2, 5)
raise TimeoutError("Service did not become healthy")
@contextmanager
def temporary_no_replicas(mode: str):
"""Context manager to temporarily remove replicas and restore them after test."""
client = globals()[f"{mode}_client"]()
try:
delete_replicas(client, mode)
wait_for_health(client)
yield
finally:
reapply_compose(COMPOSE_PATHS[mode])
wait_for_health(client)
def test_get_replica_connections_cluster_no_replicas():
with temporary_no_replicas('cluster'):
c = cluster_client()
with pytest.raises(ConnectionError, match="Unable to get cluster nodes"):
c.get_replica_connections()
def test_get_replica_connections_sentinel_no_replicas():
with temporary_no_replicas('sentinel'):
c = sentinel_client()
with pytest.raises(ConnectionError, match="Unable to get replica hostname."):
c.get_replica_connections()
docker/sentinel-compose.yml
Outdated
| sentinel-1: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-2: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-2 | ||
| hostname: sentinel-2 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26380:26380 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-2" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26380" >> /data/sentinel.conf | ||
| echo "port 26380" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-3: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-3 | ||
| hostname: sentinel-3 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26381:26381 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-3" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26381" >> /data/sentinel.conf | ||
| echo "port 26381" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks and improve reliability for sentinel services.
The sentinel services need several improvements for production readiness:
- Missing health checks for monitoring service health
- No dependencies defined on Redis master
- Runtime configuration instead of mounted configs
Apply these changes to all sentinel services:
sentinel-1:
image: falkordb/falkordb:latest
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - ./sentinel1.conf:/data/sentinel.conf:ro
networks:
- redis-sentinel
ports:
- 26379:26379
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/sentinel.conf
- echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf
- echo "sentinel announce-hostnames yes" >> /data/sentinel.conf
- echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf
- echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf
- echo "sentinel announce-port 26379" >> /data/sentinel.conf
- echo "port 26379" >> /data/sentinel.conf
- redis-server /data/sentinel.conf --sentinelCreate external configuration files for better maintainability:
# sentinel1.conf
port 26379
sentinel monitor master redis-server-1 6380 2
sentinel announce-hostnames yes
sentinel announce-ip sentinel-1
sentinel resolve-hostnames yes
sentinel announce-port 26379
🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-1: | ||
| image: falkordb/falkordb:latest | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis master server configuration.
The Redis master server configuration needs several improvements for production readiness:
- Missing health check
- No restart policy
- No volume for data persistence
- Runtime configuration instead of mounted configs
Apply these changes:
redis-server-1:
image: falkordb/falkordb:latest
hostname: redis-server-1
container_name: redis-server-1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6380", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - redis_master_data:/data
+ - ./master.conf:/data/node.conf:ro
ports:
- 6380:6380
networks:
- redis-sentinel
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/node.conf
- echo "port 6380" >> /data/node.conf
- echo "replica-announce-ip redis-server-1" >> /data/node.conf
- echo "replica-announce-port 6380" >> /data/node.conf
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- redis-server /data/node.confAdd at the end of the file:
volumes:
redis_master_data:Create external configuration file:
# master.conf
port 6380
replica-announce-ip redis-server-1
replica-announce-port 6380
loadmodule /FalkorDB/bin/src/falkordb.so
🧰 Tools
🪛 yamllint (1.35.1)
[error] 81-81: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
Outdated
| node0: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 2s | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 5000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node0" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
Fix cluster initialization and health check logic.
Critical issues in node0 configuration:
- Cluster creation in healthcheck is incorrect
- Missing restart policy
- No proper health verification
- Runtime configuration instead of mounted configs
Apply these changes:
node0:
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: always
ports:
- 5000:5000
networks:
- redis-cluster
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 2s
+ test: ["CMD", "redis-cli", "-p", "5000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - ./node0.conf:/data/node.conf:ro
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/node.conf
- echo "port 5000" >> /data/node.conf
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf
- echo "cluster-port 16379" >> /data/node.conf
- echo "cluster-enabled yes" >> /data/node.conf
- echo "cluster-config-file /data/nodes.conf" >> /data/node.conf
- echo "cluster-node-timeout 5000" >> /data/node.conf
- echo "cluster-announce-hostname node0" >> /data/node.conf
- redis-server /data/node.confAdd a new service for cluster initialization:
cluster-init:
image: falkordb/falkordb:latest
command: >
/bin/bash -c "
echo 'Waiting for nodes to be ready...' &&
sleep 10 &&
redis-cli --cluster create node0:5000 node1:6000 node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes
"
networks:
- redis-cluster
depends_on:
node0:
condition: service_healthy
node1:
condition: service_healthy
node2:
condition: service_healthy
node3:
condition: service_healthy
node4:
condition: service_healthy
node5:
condition: service_healthyCreate external configuration file:
# node0.conf
port 5000
loadmodule /FalkorDB/bin/src/falkordb.so
cluster-preferred-endpoint-type hostname
cluster-port 16379
cluster-enabled yes
cluster-config-file /data/nodes.conf
cluster-node-timeout 5000
cluster-announce-hostname node0
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 10
(indentation)
docker/cluster-compose.yml
Outdated
| - | | ||
| touch /data/node.conf | ||
| echo "port 6000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node1" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node2: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node2 | ||
| hostname: node2 | ||
| ports: | ||
| - 7000:7000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 7000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node2" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node3: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node3 | ||
| hostname: node3 | ||
| ports: | ||
| - 8000:8000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 8000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node3" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
| node4: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node4 | ||
| hostname: node4 | ||
| ports: | ||
| - 9000:9000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 9000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node4" >> /data/node.conf | ||
| redis-server /data/node.conf | ||
|
|
||
| node5: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node5 | ||
| hostname: node5 | ||
| ports: | ||
| - 10000:10000 | ||
| networks: | ||
| - redis-cluster | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 10000" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf | ||
| echo "cluster-port 16379" >> /data/node.conf | ||
| echo "cluster-enabled yes" >> /data/node.conf | ||
| echo "cluster-config-file /data/nodes.conf" >> /data/node.conf | ||
| echo "cluster-node-timeout 5000" >> /data/node.conf | ||
| echo "cluster-announce-hostname node5" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent configuration across all nodes.
The remaining nodes (node1 through node5) need the same improvements as node0:
- Missing health checks
- No restart policies
- Runtime configuration instead of mounted configs
Apply these changes to all nodes (example for node1):
node1:
image: falkordb/falkordb:latest
container_name: node1
hostname: node1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - ./node1.conf:/data/node.conf:ro
ports:
- 6000:6000
networks:
- redis-cluster
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/node.conf
- echo "port 6000" >> /data/node.conf
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- echo "cluster-preferred-endpoint-type hostname" >> /data/node.conf
- echo "cluster-port 16379" >> /data/node.conf
- echo "cluster-enabled yes" >> /data/node.conf
- echo "cluster-config-file /data/nodes.conf" >> /data/node.conf
- echo "cluster-node-timeout 5000" >> /data/node.conf
- echo "cluster-announce-hostname node1" >> /data/node.conf
- redis-server /data/node.confCreate external configuration files for each node (example for node1):
# node1.conf
port 6000
loadmodule /FalkorDB/bin/src/falkordb.so
cluster-preferred-endpoint-type hostname
cluster-port 16379
cluster-enabled yes
cluster-config-file /data/nodes.conf
cluster-node-timeout 5000
cluster-announce-hostname node1
Apply similar changes to node2 through node5, adjusting the port numbers and hostnames accordingly.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tests/test_get_replica_connections.py (2)
19-26: Improve client initialization functions.The client initialization functions have several issues:
- Code duplication in initialization pattern
- Missing docstrings and type hints
- Hardcoded connection parameters make tests less maintainable
Consider this improved implementation:
from typing import Callable from dataclasses import dataclass, field @dataclass class ClientConfig: host: str = 'localhost' ports: dict[str, int] = field(default_factory=lambda: { 'cluster': CLUSTER_PORT, 'standalone': STANDALONE_PORT, 'sentinel': SENTINEL_PORT }) def create_client(mode: str) -> Callable[[], FalkorDB]: """Create a FalkorDB client factory function for the specified mode. Args: mode: One of 'cluster', 'standalone', or 'sentinel' Returns: A factory function that creates a configured FalkorDB client """ config = ClientConfig() def client_factory() -> FalkorDB: return FalkorDB(host=config.host, port=config.ports[mode]) return client_factory cluster_client = create_client('cluster') standalone_client = create_client('standalone') sentinel_client = create_client('sentinel')
34-39: Improve test robustness and maintainability.The test has several issues:
- Hardcoded node configurations make it brittle
- Missing setup/teardown to ensure consistent cluster state
- Limited validation of connection properties
Consider using a fixture to manage cluster state:
@pytest.fixture def cluster_nodes(): return [ ('node3', 8000), ('node4', 9000), ('node5', 10000) ] def test_get_replica_connections_cluster(cluster_nodes): c = cluster_client() result = c.get_replica_connections() # Verify all expected nodes are present for node in cluster_nodes: assert node in result # Verify no unexpected nodes assert len(result) == len(cluster_nodes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/sentinel-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/sentinel-compose.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
44-44: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (1)
docker/sentinel-compose.yml (1)
113-114: LGTM!
The network configuration is minimal but sufficient for connecting the Redis and Sentinel services.
| def delete_replicas(client): | ||
| result = client.get_replica_connections() | ||
| for i in result: | ||
| name = i[0] | ||
| delete_container(name) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in delete_replicas function.
The function lacks proper error handling and type safety.
Apply these improvements:
import logging
from typing import Optional, List, Tuple
def delete_replicas(client: FalkorDB) -> None:
"""Delete replica containers.
Args:
client: FalkorDB client instance
"""
try:
result = client.get_replica_connections()
except Exception:
logging.exception("Failed to get replica connections")
return
for name, _ in result:
try:
delete_container(name)
except Exception:
logging.exception(f"Failed to delete container {name}")| def test_get_replica_connections_cluster_no_replicas(): | ||
| # assume this cluster has no replicas configured | ||
| delete_replicas(cluster_client()) | ||
| time.sleep(15) | ||
| c = cluster_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get cluster nodes"): | ||
| c.get_replica_connections() | ||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/cluster-compose.yml') | ||
|
|
||
|
|
||
| def test_get_replica_connections_sentinel_no_replicas(): | ||
| # Assume this Sentinel setup has no replicas | ||
| delete_replicas(sentinel_client()) | ||
| time.sleep(40) | ||
| c = sentinel_client() | ||
| with pytest.raises(ConnectionError, match="Unable to get replica hostname."): | ||
| c.get_replica_connections() | ||
|
|
||
| reapply_compose('/home/runner/work/falkordb-py/falkordb-py/docker/sentinel-compose.yml') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sleep with health checks and improve test reliability.
The tests have several issues:
- Hardcoded sleep times (15s and 40s) can make tests flaky
- No cleanup in case of test failure
- Duplicate code pattern
Consider this improved implementation:
from contextlib import contextmanager
def wait_for_health(client: FalkorDB, timeout: int = 30) -> None:
"""Wait for service to be healthy with exponential backoff."""
start = time.time()
delay = 1
while time.time() - start < timeout:
try:
client.ping()
return
except Exception:
time.sleep(delay)
delay = min(delay * 2, 5)
raise TimeoutError("Service did not become healthy")
@contextmanager
def temporary_no_replicas(mode: str):
"""Context manager to temporarily remove replicas and restore them."""
client = globals()[f"{mode}_client"]()
compose_file = f'/home/runner/work/falkordb-py/falkordb-py/docker/{mode}-compose.yml'
try:
delete_replicas(client)
wait_for_health(client)
yield
finally:
reapply_compose(compose_file)
wait_for_health(client)
def test_get_replica_connections_cluster_no_replicas():
with temporary_no_replicas('cluster'):
c = cluster_client()
with pytest.raises(ConnectionError, match="Unable to get cluster nodes"):
c.get_replica_connections()
def test_get_replica_connections_sentinel_no_replicas():
with temporary_no_replicas('sentinel'):
c = sentinel_client()
with pytest.raises(ConnectionError, match="Unable to get replica hostname."):
c.get_replica_connections()| def test_get_replica_connections_standalone(): | ||
| c = standalone_client() | ||
| with pytest.raises(ValueError, match="Unsupported Redis mode"): | ||
| result = c.get_replica_connections() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable assignment.
The result variable is assigned but never used.
def test_get_replica_connections_standalone():
c = standalone_client()
with pytest.raises(ValueError, match="Unsupported Redis mode"):
- result = c.get_replica_connections()
+ c.get_replica_connections()📝 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.
| def test_get_replica_connections_standalone(): | |
| c = standalone_client() | |
| with pytest.raises(ValueError, match="Unsupported Redis mode"): | |
| result = c.get_replica_connections() | |
| def test_get_replica_connections_standalone(): | |
| c = standalone_client() | |
| with pytest.raises(ValueError, match="Unsupported Redis mode"): | |
| c.get_replica_connections() |
🧰 Tools
🪛 Ruff (0.8.2)
44-44: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
| def delete_container(container_name): | ||
| subprocess.run(f"docker rm {container_name} -f", shell=True, check=True) | ||
|
|
||
| # shell=True is used to run the command in a shell | ||
| # encoding='utf-8' is used to get the output as a string | ||
| def reapply_compose(path): | ||
| subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True) |
There was a problem hiding this comment.
Security: Improve subprocess command execution.
The current implementation has potential security vulnerabilities:
- Using
shell=Truewith f-strings can lead to command injection - Missing input validation
- Basic error handling
Apply these security improvements:
def delete_container(container_name):
+ if not isinstance(container_name, str) or not container_name.isalnum():
+ raise ValueError("Invalid container name")
- subprocess.run(f"docker rm {container_name} -f", shell=True, check=True)
+ subprocess.run(["docker", "rm", container_name, "-f"],
+ check=True, shell=False, encoding='utf-8')
def reapply_compose(path):
+ if not isinstance(path, str) or '..' in path:
+ raise ValueError("Invalid path")
- subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True)
+ try:
+ subprocess.run(["docker-compose", "-f", path, "down"],
+ check=True, shell=False, encoding='utf-8')
+ subprocess.run(["docker-compose", "-f", path, "up", "-d"],
+ check=True, shell=False, encoding='utf-8')
+ except subprocess.CalledProcessError as e:
+ raise RuntimeError(f"Docker compose operation failed: {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.
| def delete_container(container_name): | |
| subprocess.run(f"docker rm {container_name} -f", shell=True, check=True) | |
| # shell=True is used to run the command in a shell | |
| # encoding='utf-8' is used to get the output as a string | |
| def reapply_compose(path): | |
| subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True) | |
| def delete_container(container_name): | |
| if not isinstance(container_name, str) or not container_name.isalnum(): | |
| raise ValueError("Invalid container name") | |
| subprocess.run(["docker", "rm", container_name, "-f"], | |
| check=True, shell=False, encoding='utf-8') | |
| # shell=True is used to run the command in a shell | |
| # encoding='utf-8' is used to get the output as a string | |
| def reapply_compose(path): | |
| if not isinstance(path, str) or '..' in path: | |
| raise ValueError("Invalid path") | |
| try: | |
| subprocess.run(["docker-compose", "-f", path, "down"], | |
| check=True, shell=False, encoding='utf-8') | |
| subprocess.run(["docker-compose", "-f", path, "up", "-d"], | |
| check=True, shell=False, encoding='utf-8') | |
| except subprocess.CalledProcessError as e: | |
| raise RuntimeError(f"Docker compose operation failed: {e}") |
docker/sentinel-compose.yml
Outdated
| sentinel-1: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "sentinel down-after-milliseconds master 1000" >> /data/sentinel.conf | ||
| echo "sentinel failover-timeout master 1000" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-2: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-2 | ||
| hostname: sentinel-2 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26380:26380 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-2" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26380" >> /data/sentinel.conf | ||
| echo "sentinel down-after-milliseconds master 1000" >> /data/sentinel.conf | ||
| echo "sentinel failover-timeout master 1000" >> /data/sentinel.conf | ||
| echo "port 26380" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel | ||
|
|
||
| sentinel-3: | ||
| image: falkordb/falkordb:latest | ||
| container_name: sentinel-3 | ||
| hostname: sentinel-3 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26381:26381 | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/sentinel.conf | ||
| echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf | ||
| echo "sentinel announce-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-ip sentinel-3" >> /data/sentinel.conf | ||
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26381" >> /data/sentinel.conf | ||
| echo "sentinel down-after-milliseconds master 1000" >> /data/sentinel.conf | ||
| echo "sentinel failover-timeout master 1000" >> /data/sentinel.conf | ||
| echo "port 26381" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve sentinel services configuration and reliability.
The sentinel services need several improvements:
- Missing health checks
- No dependencies on Redis master
- Runtime configuration instead of mounted configs
- Potential security issues with runtime commands
Apply these improvements to all sentinel services:
sentinel-1:
image: falkordb/falkordb:latest
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - ./sentinel1.conf:/data/sentinel.conf:ro
networks:
- redis-sentinel
ports:
- 26379:26379
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/sentinel.conf
- echo "sentinel monitor master redis-server-1 6380 2" >> /data/sentinel.conf
- echo "sentinel announce-hostnames yes" >> /data/sentinel.conf
- echo "sentinel announce-ip sentinel-1" >> /data/sentinel.conf
- echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf
- echo "sentinel announce-port 26379" >> /data/sentinel.conf
- echo "sentinel down-after-milliseconds master 1000" >> /data/sentinel.conf
- echo "sentinel failover-timeout master 1000" >> /data/sentinel.conf
- echo "port 26379" >> /data/sentinel.conf
- redis-server /data/sentinel.conf --sentinelCreate external configuration files:
# sentinel1.conf
port 26379
sentinel monitor master redis-server-1 6380 2
sentinel announce-hostnames yes
sentinel announce-ip sentinel-1
sentinel resolve-hostnames yes
sentinel announce-port 26379
sentinel down-after-milliseconds master 1000
sentinel failover-timeout master 1000
Apply similar changes to sentinel-2 and sentinel-3 with their respective ports.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-1: | ||
| image: falkordb/falkordb:latest | ||
| hostname: redis-server-1 | ||
| container_name: redis-server-1 | ||
| ports: | ||
| - 6380:6380 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6380" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-1" >> /data/node.conf | ||
| echo "replica-announce-port 6380" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis master server configuration and reliability.
The Redis master configuration needs several improvements:
- Missing health check
- No restart policy
- No volume for data persistence
- Runtime configuration instead of mounted configs
Apply these improvements:
redis-server-1:
image: falkordb/falkordb:latest
hostname: redis-server-1
container_name: redis-server-1
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6380", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ volumes:
+ - redis_master_data:/data
+ - ./master.conf:/data/node.conf:ro
ports:
- 6380:6380
networks:
- redis-sentinel
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/node.conf
- echo "port 6380" >> /data/node.conf
- echo "replica-announce-ip redis-server-1" >> /data/node.conf
- echo "replica-announce-port 6380" >> /data/node.conf
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- redis-server /data/node.confAdd at the end of the file:
volumes:
redis_master_data:Create external configuration file:
# master.conf
port 6380
replica-announce-ip redis-server-1
replica-announce-port 6380
loadmodule /FalkorDB/bin/src/falkordb.so
🧰 Tools
🪛 yamllint (1.35.1)
[error] 87-87: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| redis-server-2: | ||
| image: falkordb/falkordb:latest | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| entrypoint: /bin/bash | ||
| command: | ||
| - -c | ||
| - | | ||
| touch /data/node.conf | ||
| echo "port 6381" >> /data/node.conf | ||
| echo "replica-announce-ip redis-server-2" >> /data/node.conf | ||
| echo "replica-announce-port 6381" >> /data/node.conf | ||
| echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf | ||
| echo "replicaof redis-server-1 6380" >> /data/node.conf | ||
| redis-server /data/node.conf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis replica server configuration and reliability.
The Redis replica configuration needs several improvements:
- Missing health check
- No restart policy
- No volume for data persistence
- No explicit dependency on master
- Runtime configuration instead of mounted configs
Apply these improvements:
redis-server-2:
image: falkordb/falkordb:latest
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - redis_replica_data:/data
+ - ./replica.conf:/data/node.conf:ro
ports:
- 6381:6381
networks:
- redis-sentinel
- entrypoint: /bin/bash
- command:
- - -c
- - |
- touch /data/node.conf
- echo "port 6381" >> /data/node.conf
- echo "replica-announce-ip redis-server-2" >> /data/node.conf
- echo "replica-announce-port 6381" >> /data/node.conf
- echo "loadmodule /FalkorDB/bin/src/falkordb.so" >> /data/node.conf
- echo "replicaof redis-server-1 6380" >> /data/node.conf
- redis-server /data/node.confAdd to volumes section:
redis_replica_data:Create external configuration file:
# replica.conf
port 6381
replica-announce-ip redis-server-2
replica-announce-port 6381
loadmodule /FalkorDB/bin/src/falkordb.so
replicaof redis-server-1 6380
🧰 Tools
🪛 yamllint (1.35.1)
[error] 106-106: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml
Outdated
| echo "sentinel resolve-hostnames yes" >> /data/sentinel.conf | ||
| echo "sentinel announce-port 26379" >> /data/sentinel.conf | ||
| echo "port 26379" >> /data/sentinel.conf | ||
| redis-server /data/sentinel.conf --sentinel |
There was a problem hiding this comment.
We would prefer the entry point to be redis-server /data/sentinel.conf --sentinel
docker/cluster-compose.yml
Outdated
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 2s | ||
| entrypoint: /bin/bash |
There was a problem hiding this comment.
if possible change the entry point to be redis-server /data/node.conf
| def delete_container(container_name): | ||
| subprocess.run(["docker","rm",container_name,'-f'],check=True,encoding='utf-8') |
There was a problem hiding this comment.
Consider using https://docker-py.readthedocs.io/en/stable/ for all Docker related work.
|
|
||
| def test_get_replica_connections_cluster_no_replicas(): | ||
| # Assume this cluster has no replicas configured | ||
| delete_replicas(cluster_client(),'cluster') |
There was a problem hiding this comment.
Instead of deleting the Docker container, use SLAVEOF NOONE to disconnect the slave from its master.
This should work on both Sentinel and Cluster.
…1-add-support-to-get-replica-connections
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
.github/workflows/test.yml (1)
60-61: Pin Docker module version.For reproducible builds, the Docker module version should be pinned.
Apply this diff:
- name: install docker module for python - run: pip install docker + run: pip install docker==6.1.3tests/test_get_replica_connections.py (2)
1-6: Remove unused import and add type hints.The
subprocessmodule is imported but never used. Also, type hints would improve code maintainability.Apply this diff:
-import subprocess import time import pytest from falkordb import FalkorDB import docker +from typing import List, Tuple, Optional🧰 Tools
🪛 Ruff (0.8.2)
3-3:
subprocessimported but unusedRemove unused import:
subprocess(F401)
57-60: Remove unused variable assignment.The
resultvariable is assigned but never used in the test.Apply this diff:
def test_get_replica_connections_standalone(): c = standalone_client() with pytest.raises(ValueError, match="Unsupported Redis mode"): - result = c.get_replica_connections() + c.get_replica_connections()🧰 Tools
🪛 Ruff (0.8.2)
60-60: Local variable
resultis assigned to but never usedRemove assignment to unused variable
result(F841)
falkordb/falkordb.py (2)
247-249: Make empty list handling consistentThe empty list handling differs between Sentinel and Cluster modes. In both cases, we should return an empty list directly without additional checks.
- if not replica_hostnames: - return replica_hostnames return [(host, int(port)) for host, port in replica_hostnames] - if not data: - return data return [ (i.host, i.port) for i in data]Also applies to: 256-258
251-251: Improve error messages and string formattingThe error messages could be more descriptive, and there's an f-string without placeholders.
- raise ConnectionError("Failed to get replica hostnames, no hostnames found.") from e + raise ConnectionError(f"Failed to discover replicas in Sentinel mode: {str(e)}") from e - raise ConnectionError("Failed to get replica hostnames") from e + raise ConnectionError(f"Failed to discover replicas in Cluster mode: {str(e)}") from e - raise ValueError(f"Unsupported Redis mode") + raise ValueError("Unsupported Redis mode")Also applies to: 260-260, 263-263
docker/cluster-compose.yml (2)
20-21: Improve configuration management using YAML anchorsThe Redis configuration is duplicated across all nodes. Use YAML anchors to reduce duplication and improve maintainability.
x-redis-base: &redis-base cluster-enabled: "yes" cluster-port: "16379" cluster-preferred-endpoint-type: "hostname" cluster-config-file: "/data/nodes.conf" cluster-node-timeout: "500" services: node0: environment: REDIS_ARGS: >- --port 5000 --cluster-announce-hostname node0 <<: *redis-baseAlso applies to: 33-34, 46-47, 68-69, 90-91, 112-113
115-124: Enhance network and volume configurationsAdd labels and configure volume drivers for better manageability and persistence.
networks: redis-cluster: + labels: + com.falkordb.description: "FalkorDB cluster network" + driver: bridge + ipam: + driver: default volumes: node0-data: + labels: + com.falkordb.node: "node0" + driver: local + driver_opts: + type: none + device: ${PWD}/data/node0 + o: bind🧰 Tools
🪛 yamllint (1.35.1)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(4 hunks)docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)falkordb/falkordb.py(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 74-74: too many spaces after colon
(colons)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
[warning] 87-87: too many spaces after colon
(colons)
🪛 Ruff (0.8.2)
falkordb/falkordb.py
253-253: Is_Cluster may be undefined, or defined from star imports
(F405)
263-263: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (3)
.github/workflows/test.yml (1)
20-34: 🛠️ Refactor suggestion
Add health checks before running tests.
The workflow should wait for the Redis services to be healthy before proceeding with tests to prevent race conditions.
Apply this diff:
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
+ - name: Wait for standalone Redis to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done'
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel Redis to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec redis-server-1 redis-cli -p 6380 ping; do sleep 1; done'
- name: Run docker-compose for redis cluster instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/cluster-compose.yml
+ - name: Wait for cluster to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
falkordb/falkordb.py (1)
232-243: LGTM: Well-documented function
The docstring is comprehensive and clearly describes the function's purpose, return type, and possible exceptions.
docker/cluster-compose.yml (1)
12-19:
Fix healthcheck implementation
The healthcheck should verify the node's health, not create a cluster. Creating a cluster should be handled by a separate initialization service.
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 2s
+ test: ["CMD", "redis-cli", "-p", "5000", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
| def test_get_replica_connections_cluster_no_replicas(): | ||
| # assume this cluster has no replicas configured | ||
| stop_replicas(cluster_client()) | ||
| time.sleep(2) | ||
| c = cluster_client() | ||
| assert c.get_replica_connections() == [] | ||
| start_replicas(["node3", "node4", "node5"]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded sleep times with health checks.
Using fixed sleep times can lead to flaky tests. Consider using health checks instead.
Apply this diff:
+def wait_for_health(client: FalkorDB, timeout: int = 30) -> None:
+ """Wait for the service to be healthy with exponential backoff.
+
+ Args:
+ client: FalkorDB client to check
+ timeout: Maximum time to wait in seconds
+
+ Raises:
+ TimeoutError: If service does not become healthy within timeout
+ """
+ start = time.time()
+ delay = 1
+ while time.time() - start < timeout:
+ try:
+ client.ping()
+ return
+ except Exception:
+ time.sleep(delay)
+ delay = min(delay * 2, 5)
+ raise TimeoutError("Service did not become healthy")
def test_get_replica_connections_cluster_no_replicas():
stop_replicas(cluster_client())
- time.sleep(2)
+ wait_for_health(cluster_client())
c = cluster_client()
assert c.get_replica_connections() == []
start_replicas(["node3", "node4", "node5"])
+ wait_for_health(c)
def test_get_replica_connections_sentinel_no_replicas():
stop_replicas(sentinel_client())
- time.sleep(40)
+ wait_for_health(sentinel_client())
c = sentinel_client()
assert c.get_replica_connections() == []
start_replicas(["redis-server-2"])
+ wait_for_health(c)Also applies to: 77-84
| def docker_client(): | ||
| client = docker.from_env() | ||
| return client |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to docker_client function.
The function should handle potential Docker connection errors.
Apply this diff:
-def docker_client():
- client = docker.from_env()
- return client
+def docker_client() -> docker.DockerClient:
+ """Create and return a Docker client.
+
+ Returns:
+ docker.DockerClient: Initialized Docker client
+
+ Raises:
+ docker.errors.DockerException: If connection to Docker daemon fails
+ """
+ try:
+ return docker.from_env()
+ except docker.errors.DockerException as e:
+ raise RuntimeError(f"Failed to connect to Docker daemon: {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.
| def docker_client(): | |
| client = docker.from_env() | |
| return client | |
| def docker_client() -> docker.DockerClient: | |
| """Create and return a Docker client. | |
| Returns: | |
| docker.DockerClient: Initialized Docker client | |
| Raises: | |
| docker.errors.DockerException: If connection to Docker daemon fails | |
| """ | |
| try: | |
| return docker.from_env() | |
| except docker.errors.DockerException as e: | |
| raise RuntimeError(f"Failed to connect to Docker daemon: {e}") |
| def stop_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).stop() | ||
|
|
||
| def start_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).start() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve container management functions.
The container management functions need error handling and type hints.
Apply this diff:
-def stop_container(container_name):
- client = docker_client()
- client.containers.get(container_name).stop()
+def stop_container(container_name: str) -> None:
+ """Stop a Docker container.
+
+ Args:
+ container_name: Name of the container to stop
+
+ Raises:
+ RuntimeError: If container not found or stop operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.stop(timeout=10)
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to stop container {container_name}: {e}")
-def start_container(container_name):
- client = docker_client()
- client.containers.get(container_name).start()
+def start_container(container_name: str) -> None:
+ """Start a Docker container.
+
+ Args:
+ container_name: Name of the container to start
+
+ Raises:
+ RuntimeError: If container not found or start operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.start()
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to start container {container_name}: {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.
| def stop_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).stop() | |
| def start_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).start() | |
| def stop_container(container_name: str) -> None: | |
| """Stop a Docker container. | |
| Args: | |
| container_name: Name of the container to stop | |
| Raises: | |
| RuntimeError: If container not found or stop operation fails | |
| """ | |
| try: | |
| client = docker_client() | |
| container = client.containers.get(container_name) | |
| container.stop(timeout=10) | |
| except docker.errors.NotFound: | |
| raise RuntimeError(f"Container {container_name} not found") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to stop container {container_name}: {e}") | |
| def start_container(container_name: str) -> None: | |
| """Start a Docker container. | |
| Args: | |
| container_name: Name of the container to start | |
| Raises: | |
| RuntimeError: If container not found or start operation fails | |
| """ | |
| try: | |
| client = docker_client() | |
| container = client.containers.get(container_name) | |
| container.start() | |
| except docker.errors.NotFound: | |
| raise RuntimeError(f"Container {container_name} not found") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to start container {container_name}: {e}") |
docker/sentinel-compose.yml
Outdated
| redis-server-2: | ||
| image: falkordb/falkordb:latest | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| environment: | ||
| REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-1 --replica-announce-port 6381 --replicaof redis-server-1 6380" | ||
| BROWSER: 0 |
There was a problem hiding this comment.
Fix replica configuration and improve reliability.
The Redis replica has incorrect announce IP and needs similar improvements as the master.
Apply this diff:
redis-server-2:
image: falkordb/falkordb:latest
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - redis_replica_data:/data
ports:
- 6381:6381
networks:
- redis-sentinel
environment:
- REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-1 --replica-announce-port 6381 --replicaof redis-server-1 6380"
+ REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-2 --replica-announce-port 6381 --replicaof redis-server-1 6380"Add to volumes section:
redis_replica_data:🧰 Tools
🪛 yamllint (1.35.1)
[warning] 87-87: too many spaces after colon
(colons)
| sentinel-1: | ||
| image: bitnami/redis-sentinel | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| environment: | ||
| REDIS_MASTER_SET: master | ||
| REDIS_MASTER_HOST: redis-server-1 | ||
| REDIS_MASTER_PORT_NUMBER: 6380 | ||
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-1 | ||
| REDIS_SENTINEL_PORT_NUMBER: 26379 | ||
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | ||
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | ||
| ALLOW_EMPTY_PASSWORD: "yes" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks for sentinel services.
The sentinel services should have health checks to ensure they're properly initialized.
Apply this diff to all sentinel services:
sentinel-1:
image: bitnami/redis-sentinel
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
networks:
- redis-sentinel📝 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.
| sentinel-1: | |
| image: bitnami/redis-sentinel | |
| container_name: sentinel-1 | |
| hostname: sentinel-1 | |
| restart: always | |
| networks: | |
| - redis-sentinel | |
| ports: | |
| - 26379:26379 | |
| environment: | |
| REDIS_MASTER_SET: master | |
| REDIS_MASTER_HOST: redis-server-1 | |
| REDIS_MASTER_PORT_NUMBER: 6380 | |
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | |
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | |
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-1 | |
| REDIS_SENTINEL_PORT_NUMBER: 26379 | |
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | |
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | |
| ALLOW_EMPTY_PASSWORD: "yes" | |
| sentinel-1: | |
| image: bitnami/redis-sentinel | |
| container_name: sentinel-1 | |
| hostname: sentinel-1 | |
| restart: always | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "26379", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 3 | |
| networks: | |
| - redis-sentinel | |
| ports: | |
| - 26379:26379 | |
| environment: | |
| REDIS_MASTER_SET: master | |
| REDIS_MASTER_HOST: redis-server-1 | |
| REDIS_MASTER_PORT_NUMBER: 6380 | |
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | |
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | |
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-1 | |
| REDIS_SENTINEL_PORT_NUMBER: 26379 | |
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | |
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | |
| ALLOW_EMPTY_PASSWORD: "yes" |
| """ | ||
| if hasattr(self, 'sentinel') and self.sentinel is not None: | ||
| try: | ||
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated Redis terminology
The discover_slaves method is deprecated. Use discover_replicas instead to align with current Redis terminology.
- replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
+ replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)📝 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.
| replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name) | |
| replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name) |
| image: falkordb/falkordb:latest | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node0-data:/data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add restart policy and improve configuration
The node configuration should include a restart policy and use named volumes for better data persistence.
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: unless-stopped
ports:
- 5000:5000
networks:
- redis-cluster
volumes:
- - node0-data:/data
+ - type: volume
+ source: node0-data
+ target: /data📝 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.
| image: falkordb/falkordb:latest | |
| container_name: node0 | |
| hostname: node0 | |
| ports: | |
| - 5000:5000 | |
| networks: | |
| - redis-cluster | |
| volumes: | |
| - node0-data:/data | |
| image: falkordb/falkordb:latest | |
| container_name: node0 | |
| hostname: node0 | |
| restart: unless-stopped | |
| ports: | |
| - 5000:5000 | |
| networks: | |
| - redis-cluster | |
| volumes: | |
| - type: volume | |
| source: node0-data | |
| target: /data |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_get_replica_connections.py (3)
1-10: Remove unused import.The
subprocessmodule is imported but never used since thereapply_composefunction is commented out.import time import pytest -import subprocess from falkordb import FalkorDB import docker🧰 Tools
🪛 Ruff (0.8.2)
3-3:
subprocessimported but unusedRemove unused import:
subprocess(F401)
30-37: Improve client initialization functions.Consider using a factory pattern with proper type hints and configuration management.
+from typing import Callable +from dataclasses import dataclass, field +@dataclass +class ClientConfig: + host: str = 'localhost' + ports: dict[str, int] = field(default_factory=lambda: { + 'cluster': CLUSTER_PORT, + 'standalone': STANDALONE_PORT, + 'sentinel': SENTINEL_PORT + }) +def create_client(mode: str) -> Callable[[], FalkorDB]: + """Create a FalkorDB client factory function for the specified mode.""" + config = ClientConfig() + + def client_factory() -> FalkorDB: + return FalkorDB( + host=config.host, + port=config.ports[mode] + ) + return client_factory -def cluster_client(): - return FalkorDB(host='localhost', port=CLUSTER_PORT) +cluster_client = create_client('cluster') -def standalone_client(): - return FalkorDB(host='localhost', port=STANDALONE_PORT) +standalone_client = create_client('standalone') -def sentinel_client(): - return FalkorDB(host='localhost', port=SENTINEL_PORT) +sentinel_client = create_client('sentinel')
56-59: Remove unused variable assignment.The
resultvariable is assigned but never used in the assertion.def test_get_replica_connections_standalone(): c = standalone_client() with pytest.raises(ValueError, match="Unsupported Redis mode"): - result = c.get_replica_connections() + c.get_replica_connections()🧰 Tools
🪛 Ruff (0.8.2)
59-59: Local variable
resultis assigned to but never usedRemove assignment to unused variable
result(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
59-59: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (3)
tests/test_get_replica_connections.py (3)
12-14: 🛠️ Refactor suggestion
Add error handling and type hints to docker_client function.
The function should handle potential Docker connection errors and include proper type hints.
-def docker_client():
- client = docker.from_env()
- return client
+def docker_client() -> docker.DockerClient:
+ """Create and return a Docker client.
+
+ Returns:
+ docker.DockerClient: Initialized Docker client
+
+ Raises:
+ docker.errors.DockerException: If connection to Docker daemon fails
+ """
+ try:
+ return docker.from_env()
+ except docker.errors.DockerException as e:
+ raise RuntimeError(f"Failed to connect to Docker daemon: {e}")Likely invalid or redundant comment.
66-83: 🛠️ Refactor suggestion
Replace sleep with health checks.
Using fixed sleep times can lead to flaky tests. Implement proper health checks with exponential backoff.
+def wait_for_health(client: FalkorDB, timeout: int = 30) -> None:
+ """Wait for service to be healthy with exponential backoff."""
+ start = time.time()
+ delay = 1
+ while time.time() - start < timeout:
+ try:
+ client.ping()
+ return
+ except Exception:
+ time.sleep(delay)
+ delay = min(delay * 2, 5)
+ raise TimeoutError("Service did not become healthy")
def test_get_replica_connections_cluster_no_replicas():
stop_replicas(cluster_client())
- time.sleep(2)
+ wait_for_health(cluster_client())
c = cluster_client()
assert c.get_replica_connections() == []
start_replicas(["node3", "node4", "node5"])
+ wait_for_health(c)
def test_get_replica_connections_sentinel_no_replicas():
stop_replicas(sentinel_client())
- time.sleep(2)
+ wait_for_health(sentinel_client())
c = sentinel_client()
assert c.get_replica_connections() == []
start_replicas(["redis-server-2"])
+ wait_for_health(c)Likely invalid or redundant comment.
17-23: 🛠️ Refactor suggestion
Improve container management functions.
The container management functions need error handling, type hints, and proper timeout handling.
-def stop_container(container_name):
- client = docker_client()
- client.containers.get(container_name).stop()
+def stop_container(container_name: str) -> None:
+ """Stop a Docker container.
+
+ Args:
+ container_name: Name of the container to stop
+
+ Raises:
+ RuntimeError: If container not found or stop operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.stop(timeout=10)
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to stop container {container_name}: {e}")
-def start_container(container_name):
- client = docker_client()
- client.containers.get(container_name).start()
+def start_container(container_name: str) -> None:
+ """Start a Docker container.
+
+ Args:
+ container_name: Name of the container to start
+
+ Raises:
+ RuntimeError: If container not found or start operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.start()
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to start container {container_name}: {e}")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
35-45: Optimize host file modificationsThe current implementation works but could be more efficient and maintainable.
Consider this optimization:
- - name: Add entries to the host file to correctly resolve service names - run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + - name: Add entries to the host file + run: | + cat << EOF | sudo tee -a /etc/hosts + 127.0.0.1 redis-standalone + 127.0.0.1 sentinel-1 sentinel-2 sentinel-3 + 127.0.0.1 redis-server-1 redis-server-2 + 127.0.0.1 node0 node1 node2 node3 node4 node5 + EOF
11-11: Remove trailing spacesThere are trailing spaces on lines 11, 34, and 59.
Also applies to: 34-34, 59-59
🧰 Tools
🪛 yamllint (1.35.1)
[error] 11-11: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
20-34: Add health checks for Redis services
Each Redis configuration needs a health check to ensure the service is ready before running tests.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
Line range hint 62-71: Test execution and coverage setup looks good!
The configuration for running tests and uploading coverage reports is well-structured.
🧰 Tools
🪛 actionlint (1.7.4)
68-68: property "platform" is not defined in object type {python: number}
(expression)
20-34: Verify Docker image configuration
As per the PR objectives, please ensure that the Docker compose files are using the standard FalkorDB/FalkorDB image.
✅ Verification successful
Docker images are correctly configured
The verification shows that all Redis instances are using the correct falkordb/falkordb:latest image:
- Standalone setup: Uses
falkordb/falkordb:latest - Sentinel setup: Uses
falkordb/falkordb:latestfor Redis servers andbitnami/redis-sentinelfor sentinel nodes (which is appropriate as sentinels are a different component) - Cluster setup: All nodes use
falkordb/falkordb:latest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Docker image configuration in compose files
for file in ./docker/*compose.yml; do
echo "Checking $file for FalkorDB image..."
grep -A 5 "image:" "$file"
done
Length of output: 2219
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
.github/workflows/test.yml
Outdated
| - name: install docker module for python | ||
| run: pip install docker-py |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Poetry for docker-py installation
Installing docker-py directly with pip while using Poetry for other dependencies could lead to version conflicts.
Add docker-py to your pyproject.toml instead:
- - name: install docker module for python
- run: pip install docker-py
+ - name: Add docker-py dependency
+ run: poetry add docker-py📝 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.
| - name: install docker module for python | |
| run: pip install docker-py | |
| - name: Add docker-py dependency | |
| run: poetry add docker-py |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
35-45: Improve host file modifications using a loop.The current implementation has repetitive commands. Consider using a loop for better maintainability.
Apply this diff:
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + hosts=("redis-standalone" "sentinel-{1..3}" "redis-server-{1..2}" "node{0..5}") + for pattern in "${hosts[@]}"; do + for host in $(eval echo $pattern); do + echo "127.0.0.1 $host" | sudo tee -a /etc/hosts + done + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(3 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
69-69: property "platform" is not defined in object type {python: number}
(expression)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
61-61: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (3)
.github/workflows/test.yml (2)
60-61: 🛠️ Refactor suggestion
Use Poetry for docker-py installation.
Installing docker-py directly with pip while using Poetry for other dependencies could lead to version conflicts.
Apply this diff:
- - name: install docker module for python
- run: pip install docker-py
+ - name: Add docker-py dependency
+ run: poetry add docker-pyLikely invalid or redundant comment.
20-34: 🛠️ Refactor suggestion
Add health checks for Redis services.
The workflow should wait for Redis services to be healthy before proceeding with tests.
Apply this diff:
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
+ - name: Wait for standalone Redis to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done'
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done'
- name: Run docker-compose for redis cluster instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/cluster-compose.yml
+ - name: Wait for cluster to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
tests/test_get_replica_connections.py (1)
58-61: 🛠️ Refactor suggestion
Remove unused variable assignment.
The result variable is assigned but never used.
Apply this diff:
def test_get_replica_connections_standalone():
c = standalone_client()
with pytest.raises(ValueError, match="Unsupported Redis mode"):
- result = c.get_replica_connections()
+ c.get_replica_connections()Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
61-61: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
| import time | ||
| import pytest | ||
| import subprocess | ||
| from falkordb import FalkorDB | ||
| import docker | ||
|
|
||
|
|
||
| time.sleep(10) # wait for the containers to start | ||
| CLUSTER_PORT = 5000 | ||
| STANDALONE_PORT = 6379 | ||
| SENTINEL_PORT = 26379 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up imports and improve initial wait.
The code has an unused import and a hardcoded initial wait time.
Apply this diff:
-import subprocess
import time
import pytest
from falkordb import FalkorDB
import docker
-time.sleep(10) # wait for the containers to start
+def wait_for_services():
+ """Wait for all Redis services to be ready."""
+ clients = [cluster_client(), standalone_client(), sentinel_client()]
+ for client in clients:
+ start = time.time()
+ while time.time() - start < 30:
+ try:
+ client.ping()
+ break
+ except Exception:
+ time.sleep(1)
+ else:
+ raise TimeoutError(f"Service on port {client.port} not ready")
+wait_for_services()📝 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.
| import time | |
| import pytest | |
| import subprocess | |
| from falkordb import FalkorDB | |
| import docker | |
| time.sleep(10) # wait for the containers to start | |
| CLUSTER_PORT = 5000 | |
| STANDALONE_PORT = 6379 | |
| SENTINEL_PORT = 26379 | |
| import time | |
| import pytest | |
| from falkordb import FalkorDB | |
| import docker | |
| def wait_for_services(): | |
| """Wait for all Redis services to be ready.""" | |
| clients = [cluster_client(), standalone_client(), sentinel_client()] | |
| for client in clients: | |
| start = time.time() | |
| while time.time() - start < 30: | |
| try: | |
| client.ping() | |
| break | |
| except Exception: | |
| time.sleep(1) | |
| else: | |
| raise TimeoutError(f"Service on port {client.port} not ready") | |
| wait_for_services() | |
| CLUSTER_PORT = 5000 | |
| STANDALONE_PORT = 6379 | |
| SENTINEL_PORT = 26379 |
🧰 Tools
🪛 Ruff (0.8.2)
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
| def docker_client(): | ||
| client = docker.from_env() | ||
| return client | ||
| # check=True is used to raise an exception if the command fails | ||
|
|
||
| def stop_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).stop() | ||
|
|
||
| def start_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).start() | ||
|
|
||
| # shell=True is used to run the command in a shell | ||
| # encoding='utf-8' is used to get the output as a string | ||
| # def reapply_compose(path): | ||
| # subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Docker client implementation.
The Docker client functions lack proper error handling and type hints.
Apply this diff:
-def docker_client():
- client = docker.from_env()
- return client
+def docker_client() -> docker.DockerClient:
+ """Create and return a Docker client.
+
+ Returns:
+ docker.DockerClient: Initialized Docker client
+
+ Raises:
+ RuntimeError: If connection to Docker daemon fails
+ """
+ try:
+ return docker.from_env()
+ except docker.errors.DockerException as e:
+ raise RuntimeError(f"Failed to connect to Docker daemon: {e}")
-def stop_container(container_name):
- client = docker_client()
- client.containers.get(container_name).stop()
+def stop_container(container_name: str) -> None:
+ """Stop a Docker container.
+
+ Args:
+ container_name: Name of the container to stop
+
+ Raises:
+ RuntimeError: If container not found or stop operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.stop(timeout=10)
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to stop container {container_name}: {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.
| def docker_client(): | |
| client = docker.from_env() | |
| return client | |
| # check=True is used to raise an exception if the command fails | |
| def stop_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).stop() | |
| def start_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).start() | |
| # shell=True is used to run the command in a shell | |
| # encoding='utf-8' is used to get the output as a string | |
| # def reapply_compose(path): | |
| # subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True) | |
| def docker_client() -> docker.DockerClient: | |
| """Create and return a Docker client. | |
| Returns: | |
| docker.DockerClient: Initialized Docker client | |
| Raises: | |
| RuntimeError: If connection to Docker daemon fails | |
| """ | |
| try: | |
| return docker.from_env() | |
| except docker.errors.DockerException as e: | |
| raise RuntimeError(f"Failed to connect to Docker daemon: {e}") | |
| def stop_container(container_name: str) -> None: | |
| """Stop a Docker container. | |
| Args: | |
| container_name: Name of the container to stop | |
| Raises: | |
| RuntimeError: If container not found or stop operation fails | |
| """ | |
| try: | |
| client = docker_client() | |
| container = client.containers.get(container_name) | |
| container.stop(timeout=10) | |
| except docker.errors.NotFound: | |
| raise RuntimeError(f"Container {container_name} not found") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to stop container {container_name}: {e}") | |
| def start_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).start() | |
| # shell=True is used to run the command in a shell | |
| # encoding='utf-8' is used to get the output as a string | |
| # def reapply_compose(path): | |
| # subprocess.run(f"docker-compose -f {path} down && docker-compose -f {path} up -d", check=True, shell=True) |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
docker/cluster-compose.yml (2)
12-19:⚠️ Potential issueMove cluster creation out of health check
The health check should verify the node's health, not create the cluster. This could cause issues with multiple nodes trying to create the cluster simultaneously.
Replace with a simple health check and move cluster creation to a dedicated initialization service as suggested in previous reviews.
healthcheck: - test: | - redis-cli --cluster create node0:5000 node1:6000 \ - node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 - interval: 3s - timeout: 10s - retries: 10 - start_period: 2s + test: ["CMD", "redis-cli", "-p", "5000", "ping"] + interval: 5s + timeout: 3s + retries: 3🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
23-48: 🛠️ Refactor suggestionApply consistent configuration across nodes
Nodes 1 and 2 have the same configuration issues as node0:
- Missing health checks
- No restart policies
- Using environment variables instead of mounted configs
Apply the same improvements as suggested for node0 to both nodes, adjusting ports and hostnames accordingly.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (3)
.github/workflows/test.yml (1)
35-45: Improve host file modification.The current implementation uses multiple echo commands. This can be optimized using a heredoc.
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + sudo tee -a /etc/hosts << EOF + 127.0.0.1 redis-standalone + 127.0.0.1 sentinel-1 + 127.0.0.1 sentinel-2 + 127.0.0.1 sentinel-3 + 127.0.0.1 redis-server-1 + 127.0.0.1 redis-server-2 + $(for i in {0..5}; do echo "127.0.0.1 node$i"; done) + EOFtests/test_get_replica_connections.py (1)
1-5: Clean up imports and use type hints.The code has unused imports and lacks type hints.
+from typing import List, Tuple import time import pytest -import subprocess # unused import from falkordb import FalkorDB import docker🧰 Tools
🪛 Ruff (0.8.2)
3-3:
subprocessimported but unusedRemove unused import:
subprocess(F401)
docker/cluster-compose.yml (1)
115-124: Improve network and volume configurationsThe network and volume configurations could be more robust:
- Network configuration lacks explicit settings
- Volumes lack explicit configuration
- Missing newline at end of file
Consider these improvements:
networks: redis-cluster: + driver: bridge + ipam: + driver: default + config: + - subnet: 172.28.0.0/16 volumes: node0-data: + driver: local node1-data: + driver: local node2-data: + driver: local node3-data: + driver: local node4-data: + driver: local node5-data: + driver: local +🧰 Tools
🪛 yamllint (1.35.1)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.yml(3 hunks)docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 74-74: too many spaces after colon
(colons)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
[warning] 87-87: too many spaces after colon
(colons)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
78-78: property "platform" is not defined in object type {python: number}
(expression)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (8)
.github/workflows/test.yml (1)
60-61: Use Poetry for docker-py installation.
Installing docker module directly with pip while using Poetry for other dependencies could lead to version conflicts.
tests/test_get_replica_connections.py (3)
13-15: Improve docker client initialization.
The docker client initialization lacks error handling and type hints.
40-49: Use Redis SLAVEOF command instead of container management.
As suggested by reviewer swilly22, use SLAVEOF NOONE command instead of stopping containers.
67-74: Replace sleep with health checks.
Using fixed sleep times (2s and 40s) can lead to flaky tests.
Also applies to: 77-84
docker/sentinel-compose.yml (4)
2-22: Add health checks for sentinel services.
The sentinel services should have health checks to ensure they're properly initialized.
65-76: Improve Redis master configuration.
The Redis master server needs health checks, restart policy, and volume mounts.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 74-74: too many spaces after colon
(colons)
[error] 76-76: trailing spaces
(trailing-spaces)
78-88: Fix replica configuration and improve reliability.
The Redis replica needs health checks, restart policy, and proper dependencies.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 87-87: too many spaces after colon
(colons)
21-21: Review security implications of empty passwords.
The ALLOW_EMPTY_PASSWORD: "yes" setting could pose security risks in production environments.
Also applies to: 42-42, 63-63
| - name: Run docker-compose for redis standalone instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/standalone-compose.yml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use relative paths for compose files.
The compose file paths should be relative to the repository root.
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
- compose-file: ./docker/standalone-compose.yml
+ compose-file: docker/standalone-compose.yml📝 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.
| - name: Run docker-compose for redis standalone instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/standalone-compose.yml | |
| - name: Run docker-compose for redis standalone instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: docker/standalone-compose.yml |
.github/workflows/test.yml
Outdated
| - name: Get logs from node1 | ||
| run: docker logs node1 | ||
|
|
||
| - name: Get logs from node5 | ||
| run: docker logs node5 | ||
|
|
||
| - name: Get logs from redis-server-2 | ||
| run: docker logs redis-server-2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for log retrieval.
The log retrieval steps should handle cases where containers might not exist or logs are not available.
- name: Get logs from node1
- run: docker logs node1
+ run: docker logs node1 || echo "Failed to retrieve logs from node1"
+ continue-on-error: true
- name: Get logs from node5
- run: docker logs node5
+ run: docker logs node5 || echo "Failed to retrieve logs from node5"
+ continue-on-error: true
- name: Get logs from redis-server-2
- run: docker logs redis-server-2
+ run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2"
+ continue-on-error: true📝 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.
| - name: Get logs from node1 | |
| run: docker logs node1 | |
| - name: Get logs from node5 | |
| run: docker logs node5 | |
| - name: Get logs from redis-server-2 | |
| run: docker logs redis-server-2 | |
| - name: Get logs from node1 | |
| run: docker logs node1 || echo "Failed to retrieve logs from node1" | |
| continue-on-error: true | |
| - name: Get logs from node5 | |
| run: docker logs node5 || echo "Failed to retrieve logs from node5" | |
| continue-on-error: true | |
| - name: Get logs from redis-server-2 | |
| run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2" | |
| continue-on-error: true |
| node0: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node0-data:/data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add restart policy and improve configuration management
The node configuration is missing important stability features:
- No restart policy defined
- Using environment variables for configuration instead of mounted config files
Apply these improvements:
node0:
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: unless-stopped
ports:
- 5000:5000
networks:
- redis-cluster
volumes:
- - node0-data:/data
+ - ./config/node0.conf:/data/node.conf:ro
+ - node0-data:/dataCreate a configuration file config/node0.conf:
port 5000
cluster-enabled yes
cluster-port 16379
cluster-preferred-endpoint-type hostname
cluster-config-file /data/nodes.conf
cluster-node-timeout 500
cluster-announce-hostname node0
docker/cluster-compose.yml
Outdated
| healthcheck: | ||
| test: | | ||
| #!/bin/bash | ||
| ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n') | ||
| redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 10s |
There was a problem hiding this comment.
Fix health check logic in replica nodes
The health checks in nodes 3-5 have several issues:
- Complex shell scripts in health checks
- Incorrect shell variable syntax (
$$ipinstead of$ip) - The
|| echo failed && exit 1logic is incorrect (will always exit)
Simplify the health checks:
healthcheck:
- test: |
- #!/bin/bash
- ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n')
- redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1
+ test: ["CMD", "redis-cli", "-p", "8000", "cluster", "info"]
interval: 5s
timeout: 3s
- retries: 10
- start_period: 10s
+ retries: 3Move cluster setup to a dedicated initialization service as suggested in previous reviews.
Also applies to: 81-89, 103-111
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/test.yml (2)
60-61: Move docker dependency to pyproject.tomlInstead of installing the dependency during CI, add it to
pyproject.toml.- - name: install docker module for python - run: poetry add dockerAdd to
pyproject.toml:[tool.poetry.dependencies] docker = "^6.1.3"
64-74: Add error handling for log retrievalThe log retrieval steps should handle cases where containers might not exist.
- name: Get logs from node1 - run: docker logs node0 + run: docker logs node0 || echo "Failed to retrieve logs from node0" + continue-on-error: true - name: Get logs from node1 - run: docker logs node1 + run: docker logs node1 || echo "Failed to retrieve logs from node1" + continue-on-error: true - name: Get logs from node5 - run: docker logs node5 + run: docker logs node5 || echo "Failed to retrieve logs from node5" + continue-on-error: true - name: Get logs from redis-server-2 - run: docker logs redis-server-2 + run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2" + continue-on-error: true🧰 Tools
🪛 yamllint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml (1)
118-124: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
node3-data: node4-data: - node5-data: + node5-data: +🧰 Tools
🪛 yamllint (1.35.1)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(3 hunks)docker/cluster-compose.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
80-80: property "platform" is not defined in object type {python: number}
(expression)
🔇 Additional comments (3)
.github/workflows/test.yml (1)
35-45: LGTM!
The host file modifications are implemented correctly, using both individual entries and a loop for efficiency.
docker/cluster-compose.yml (2)
115-117: LGTM!
The network configuration is correctly defined for the cluster communication.
59-111:
Fix health check logic in replica nodes
The health checks have several issues:
- Complex shell scripts in health checks
- Incorrect shell variable syntax (
$$ipinstead of$ip) - The
|| echo failed && exit 1logic is incorrect
healthcheck:
- test: |
- #!/bin/bash
- ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n')
- redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1
+ test: ["CMD", "redis-cli", "-p", "8000", "cluster", "info"]
interval: 5s
timeout: 3s
- retries: 10
- start_period: 10s
+ retries: 3Apply similar changes to node4 and node5 health checks.
Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 70-70: trailing spaces
(trailing-spaces)
| if: matrix.python == '3.10' && matrix.platform != 'macos-11' | ||
| with: | ||
| fail_ci_if_error: false | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
Fix undefined matrix variable
The platform variable is referenced but not defined in the matrix configuration.
- name: Upload coverage
uses: codecov/codecov-action@v5
- if: matrix.python == '3.10' && matrix.platform != 'macos-11'
+ if: matrix.python == '3.10'
with:
fail_ci_if_error: false
token: ${{ secrets.CODECOV_TOKEN }}📝 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.
| if: matrix.python == '3.10' && matrix.platform != 'macos-11' | |
| with: | |
| fail_ci_if_error: false | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| if: matrix.python == '3.10' | |
| with: | |
| fail_ci_if_error: false | |
| token: ${{ secrets.CODECOV_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.4)
80-80: property "platform" is not defined in object type {python: number}
(expression)
| - name: Run docker-compose for redis standalone instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/standalone-compose.yml | ||
|
|
||
| - name: Run docker-compose for redis replication instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/sentinel-compose.yml | ||
|
|
||
| - name: Run docker-compose for redis cluster instance | ||
| uses: hoverkraft-tech/[email protected] | ||
| with: | ||
| compose-file: ./docker/cluster-compose.yml | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks before proceeding with tests
The workflow should wait for the services to be healthy before proceeding with tests.
Apply this diff:
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
+ - name: Wait for standalone to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done'
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done'
- name: Run docker-compose for redis cluster instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/cluster-compose.yml
+ - name: Wait for cluster to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'📝 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.
| - name: Run docker-compose for redis standalone instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/standalone-compose.yml | |
| - name: Run docker-compose for redis replication instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/sentinel-compose.yml | |
| - name: Run docker-compose for redis cluster instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/cluster-compose.yml | |
| - name: Run docker-compose for redis standalone instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/standalone-compose.yml | |
| - name: Wait for standalone to be ready | |
| run: | | |
| timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done' | |
| - name: Run docker-compose for redis replication instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/sentinel-compose.yml | |
| - name: Wait for sentinel to be ready | |
| run: | | |
| timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done' | |
| - name: Run docker-compose for redis cluster instance | |
| uses: hoverkraft-tech/[email protected] | |
| with: | |
| compose-file: ./docker/cluster-compose.yml | |
| - name: Wait for cluster to be ready | |
| run: | | |
| timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done' |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/cluster-compose.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
docker/cluster-compose.yml (2)
49-113:
Fix health check implementation in replica nodes
The health checks in nodes 3-5 have several issues:
- Complex shell scripts in health checks
- Incorrect shell variable syntax (
$$ip) - The
|| echo failed && exit 1logic is incorrect
Example fix for node3 (apply similar changes to node4 and node5):
node3:
image: falkordb/falkordb:latest
container_name: node3
hostname: node3
+ restart: unless-stopped
ports:
- 8000:8000
networks:
- redis-cluster
volumes:
- node3-data:/data
+ - ./config/node3.conf:/data/node.conf:ro
healthcheck:
- test: |
- #!/bin/bash
- ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n')
- redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1
+ test: ["CMD", "redis-cli", "-p", "8000", "cluster", "info"]
interval: 5s
timeout: 3s
- retries: 10
- start_period: 20s
+ retries: 3
- environment:
- REDIS_ARGS: "--port 8000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node3"
+ command: redis-server /data/node.confLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 70-70: trailing spaces
(trailing-spaces)
23-48: 🛠️ Refactor suggestion
Apply consistent configuration to node1 and node2
The same improvements from node0 should be applied to these nodes:
- Add restart policy
- Use mounted configuration files
- Add health checks
Example for node1 (apply similar changes to node2):
node1:
image: falkordb/falkordb:latest
container_name: node1
hostname: node1
+ restart: unless-stopped
ports:
- 6000:6000
networks:
- redis-cluster
volumes:
- node1-data:/data
+ - ./config/node1.conf:/data/node.conf:ro
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
- environment:
- REDIS_ARGS: "--port 6000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node1"
+ command: redis-server /data/node.confLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
Outdated
| container_name: node1 | ||
| hostname: node1 | ||
| ports: | ||
| - 6000:6000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node1-data:/data | ||
| environment: | ||
| REDIS_ARGS: "--port 6000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node1" | ||
|
|
||
| node2: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node2 | ||
| hostname: node2 | ||
| ports: | ||
| - 7000:7000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node2-data:/data | ||
| environment: | ||
| REDIS_ARGS: "--port 7000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node2" | ||
|
|
||
| node3: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node3 | ||
| hostname: node3 | ||
| ports: | ||
| - 8000:8000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node3-data:/data | ||
| healthcheck: | ||
| test: | | ||
| #!/bin/bash | ||
| ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n') | ||
| redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 20s | ||
| environment: | ||
| REDIS_ARGS: "--port 8000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node3" | ||
|
|
||
| node4: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node4 | ||
| hostname: node4 | ||
| ports: | ||
| - 9000:9000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node4-data:/data | ||
| healthcheck: | ||
| test: | | ||
| #!/bin/bash | ||
| ip=$(getent hosts node0 |awk '{print $1}'| tr -d '\r\n') | ||
| redis-cli -p 9000 cluster meet $$ip 5000 || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 20s | ||
| environment: | ||
| REDIS_ARGS: "--port 9000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node4" | ||
|
|
||
| node5: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node5 | ||
| hostname: node5 | ||
| ports: | ||
| - 10000:10000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node5-data:/data | ||
| healthcheck: | ||
| test: | | ||
| #!/bin/bash | ||
| ip=$(getent hosts node1 |awk '{print $1}'| tr -d '\r\n') | ||
| redis-cli -p 10000 cluster meet $$ip 6000|| echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 20s | ||
| environment: | ||
| REDIS_ARGS: "--port 10000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node5" | ||
|
|
||
| networks: | ||
| redis-cluster: | ||
|
|
||
| volumes: | ||
| node0-data: | ||
| node1-data: | ||
| node2-data: | ||
| node3-data: | ||
| node4-data: | ||
| node5-data: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dedicated cluster initialization service
A separate service should handle cluster initialization after all nodes are healthy.
Add this service to the compose file:
cluster-init:
image: falkordb/falkordb:latest
command: >
/bin/bash -c "
echo 'Waiting for nodes to be ready...' &&
sleep 10 &&
redis-cli --cluster create node0:5000 node1:6000 node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes
"
networks:
- redis-cluster
depends_on:
node0:
condition: service_healthy
node1:
condition: service_healthy
node2:
condition: service_healthy
node3:
condition: service_healthy
node4:
condition: service_healthy
node5:
condition: service_healthyAdd a newline at the end of the file.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
tests/test_get_replica_connections.py (1)
77-84: 🛠️ Refactor suggestionApply the same health check improvements to sentinel test.
This test has the same issues with sleep times as the cluster test.
Apply the same changes as suggested for the cluster test, replacing sleep with health checks:
def test_get_replica_connections_sentinel_no_replicas(): stop_replicas(sentinel_client()) - time.sleep(2) + wait_for_replica_state(sentinel_client(), expected_empty=True) c = sentinel_client() assert c.get_replica_connections() == [] start_replicas(["redis-server-2"]) + wait_for_replica_state(c, expected_empty=False)
🧹 Nitpick comments (4)
tests/test_get_replica_connections.py (2)
31-38: Add docstrings to client factory functions.The client factory functions would benefit from proper documentation.
def cluster_client(): + """Create a FalkorDB client configured for cluster mode. + + Returns: + FalkorDB: Configured client instance + """ return FalkorDB(host='localhost', port=CLUSTER_PORT) def standalone_client(): + """Create a FalkorDB client configured for standalone mode. + + Returns: + FalkorDB: Configured client instance + """ return FalkorDB(host='localhost', port=STANDALONE_PORT) def sentinel_client(): + """Create a FalkorDB client configured for sentinel mode. + + Returns: + FalkorDB: Configured client instance + """ return FalkorDB(host='localhost', port=SENTINEL_PORT)
50-56: Use constants for port numbers in assertions.Hardcoded port numbers should be replaced with constants for better maintainability.
def test_get_replica_connections_cluster(): c = cluster_client() result = c.get_replica_connections() - assert ('node3', 8000) in result - assert ('node4', 9000) in result - assert ('node5', 10000) in result + REPLICA_PORTS = { + 'node3': 8000, + 'node4': 9000, + 'node5': 10000 + } + for node, port in REPLICA_PORTS.items(): + assert (node, port) in result.github/workflows/test.yml (1)
35-45: Optimize host file modificationsReduce the number of sudo calls by using a single command.
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + sudo tee -a /etc/hosts << EOF + 127.0.0.1 redis-standalone + 127.0.0.1 sentinel-1 + 127.0.0.1 sentinel-2 + 127.0.0.1 sentinel-3 + 127.0.0.1 redis-server-1 + 127.0.0.1 redis-server-2 + 127.0.0.1 node0 node1 node2 node3 node4 node5 + EOFdocker/cluster-compose.yml (1)
22-22: Fix formatting issuesAddress the following formatting issues:
- Remove trailing spaces
- Add newline at end of file
- + node1: ... - + node2: ... - + node3: ... - + node4: ... node5-data: +Also applies to: 35-35, 48-48, 70-70, 124-124
🧰 Tools
🪛 yamllint (1.35.1)
[error] 22-22: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(3 hunks)docker/cluster-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
82-82: property "platform" is not defined in object type {python: number}
(expression)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (7)
tests/test_get_replica_connections.py (3)
57-61: 🛠️ Refactor suggestion
Remove unused variable assignment.
The result variable is assigned but never used.
def test_get_replica_connections_standalone():
c = standalone_client()
with pytest.raises(ValueError, match="Unsupported Redis mode"):
- result = c.get_replica_connections()
+ c.get_replica_connections()Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
67-74: 🛠️ Refactor suggestion
Replace sleep with health checks and improve test reliability.
Using fixed sleep times can lead to flaky tests.
+def wait_for_replica_state(client: FalkorDB, expected_empty: bool, timeout: int = 30) -> None:
+ """Wait for replicas to reach expected state."""
+ start = time.time()
+ while time.time() - start < timeout:
+ try:
+ replicas = client.get_replica_connections()
+ if (not replicas) == expected_empty:
+ return
+ except Exception:
+ pass
+ time.sleep(1)
+ raise TimeoutError("Replicas did not reach expected state")
def test_get_replica_connections_cluster_no_replicas():
stop_replicas(cluster_client())
- time.sleep(2)
+ wait_for_replica_state(cluster_client(), expected_empty=True)
c = cluster_client()
assert c.get_replica_connections() == []
start_replicas(["node3", "node4", "node5"])
+ wait_for_replica_state(c, expected_empty=False)Likely invalid or redundant comment.
13-24: 🛠️ Refactor suggestion
Improve Docker client functions with proper error handling and type hints.
The Docker client functions lack proper error handling, type hints, and documentation.
-def docker_client():
- client = docker.from_env()
- return client
+def docker_client() -> docker.DockerClient:
+ """Create and return a Docker client.
+
+ Returns:
+ docker.DockerClient: Initialized Docker client
+
+ Raises:
+ docker.errors.DockerException: If connection to Docker daemon fails
+ """
+ try:
+ return docker.from_env()
+ except docker.errors.DockerException as e:
+ raise RuntimeError(f"Failed to connect to Docker daemon: {e}")
-def stop_container(container_name):
- client = docker_client()
- client.containers.get(container_name).stop()
+def stop_container(container_name: str) -> None:
+ """Stop a Docker container.
+
+ Args:
+ container_name: Name of the container to stop
+
+ Raises:
+ RuntimeError: If container not found or stop operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.stop(timeout=10)
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to stop container {container_name}: {e}")Likely invalid or redundant comment.
.github/workflows/test.yml (3)
82-85:
Fix undefined matrix variable
The platform variable is referenced but not defined in the matrix configuration.
- name: Upload coverage
uses: codecov/codecov-action@v5
- if: matrix.python == '3.10' && matrix.platform != 'macos-11'
+ if: matrix.python == '3.10'
with:
fail_ci_if_error: false
token: ${{ secrets.CODECOV_TOKEN }}Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
82-82: property "platform" is not defined in object type {python: number}
(expression)
66-76: 🛠️ Refactor suggestion
Add error handling for log retrieval
Add error handling to prevent workflow failures if containers are not available.
- name: Get logs from node1
- run: docker logs node0
+ run: docker logs node0 || echo "Failed to retrieve logs from node0"
+ continue-on-error: true
- name: Get logs from node1
- run: docker logs node1
+ run: docker logs node1 || echo "Failed to retrieve logs from node1"
+ continue-on-error: true
- name: Get logs from node5
- run: docker logs node5
+ run: docker logs node5 || echo "Failed to retrieve logs from node5"
+ continue-on-error: true
- name: Get logs from redis-server-2
- run: docker logs redis-server-2
+ run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2"
+ continue-on-error: trueLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 68-68: trailing spaces
(trailing-spaces)
20-34: 🛠️ Refactor suggestion
Add health checks before proceeding with tests
Add health checks after each docker-compose step to ensure services are ready before proceeding.
- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
+ - name: Wait for standalone to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done'
- name: Run docker-compose for redis replication instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/sentinel-compose.yml
+ - name: Wait for sentinel to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done'
- name: Run docker-compose for redis cluster instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/cluster-compose.yml
+ - name: Wait for cluster to be ready
+ run: |
+ timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml (1)
49-113:
Fix health check logic in replica nodes
The health checks in nodes 3-5 have several issues:
- Complex shell scripts in health checks
- Incorrect shell variable syntax (
$$ipinstead of$ip) - The
|| echo failed && exit 1logic is incorrect (will always exit)
healthcheck:
- test: |
- #!/bin/bash
- ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n')
- redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1
+ test: ["CMD", "redis-cli", "-p", "8000", "cluster", "info"]
interval: 5s
timeout: 3s
- retries: 10
- start_period: 30s
+ retries: 3Apply similar changes to nodes 4 and 5.
Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 70-70: trailing spaces
(trailing-spaces)
| import time | ||
| import pytest | ||
| import subprocess | ||
| from falkordb import FalkorDB | ||
| import docker |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import and improve imports organization.
The subprocess module is imported but never used.
import time
import pytest
-import subprocess
from falkordb import FalkorDB
import docker📝 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.
| import time | |
| import pytest | |
| import subprocess | |
| from falkordb import FalkorDB | |
| import docker | |
| import time | |
| import pytest | |
| from falkordb import FalkorDB | |
| import docker |
🧰 Tools
🪛 Ruff (0.8.2)
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
| services: | ||
| node0: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node0-data:/data | ||
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 1s | ||
| environment: | ||
| REDIS_ARGS: "--port 5000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node0" | ||
|
|
There was a problem hiding this comment.
Critical issues in node0 configuration
Several critical issues need to be addressed:
- Cluster creation should not be in health check
- Missing restart policy
- Configuration through environment variables is less maintainable
node0:
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: unless-stopped
ports:
- 5000:5000
networks:
- redis-cluster
volumes:
- node0-data:/data
+ - ./config/node0.conf:/data/node.conf:ro
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 1s
+ test: ["CMD", "redis-cli", "-p", "5000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
- environment:
- REDIS_ARGS: "--port 5000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node0"
+ command: redis-server /data/node.confCreate a configuration file config/node0.conf:
port 5000
loadmodule /FalkorDB/bin/src/falkordb.so
cluster-enabled yes
cluster-port 16379
cluster-preferred-endpoint-type hostname
cluster-config-file /data/nodes.conf
cluster-node-timeout 500
cluster-announce-hostname node0
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/test.yml (3)
20-34: 🛠️ Refactor suggestionAdd health checks for Redis services
Health checks are necessary to ensure services are ready before proceeding with tests.
Apply this diff:
- name: Run docker-compose for redis standalone instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/standalone-compose.yml + - name: Wait for standalone to be ready + run: | + timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done' - name: Run docker-compose for redis replication instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/sentinel-compose.yml + - name: Wait for sentinel to be ready + run: | + timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done' - name: Run docker-compose for redis cluster instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/cluster-compose.yml + - name: Wait for cluster to be ready + run: | + timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
60-61: 🛠️ Refactor suggestionMove docker dependency to project dependencies
Instead of installing the docker module at runtime, it should be added to the project's dependencies.
Add to your pyproject.toml:
[tool.poetry.dependencies] docker = "^6.1.3"Then remove these lines from the workflow.
82-82:⚠️ Potential issueFix undefined matrix variable
The
platformvariable is referenced but not defined in the matrix configuration.- name: Upload coverage uses: codecov/codecov-action@v5 - if: matrix.python == '3.10' && matrix.platform != 'macos-11' + if: matrix.python == '3.10'🧰 Tools
🪛 actionlint (1.7.4)
82-82: property "platform" is not defined in object type {python: number}
(expression)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
82-82: property "platform" is not defined in object type {python: number}
(expression)
🔇 Additional comments (2)
.github/workflows/test.yml (2)
35-46: LGTM! Efficient host file configuration
The host file configuration is well-structured with an efficient loop for node entries.
66-76: Add error handling for log retrieval
Log retrieval should handle cases where containers might not exist or logs are not available.
Apply this diff:
- name: Get logs from node0
- run: docker logs node0
+ run: docker logs node0 || echo "Failed to retrieve logs from node0"
+ continue-on-error: true
- name: Get logs from node1
- run: docker logs node1
+ run: docker logs node1 || echo "Failed to retrieve logs from node1"
+ continue-on-error: true
- name: Get logs from node5
- run: docker logs node5
+ run: docker logs node5 || echo "Failed to retrieve logs from node5"
+ continue-on-error: true
- name: Get logs from redis-server-2
- run: docker logs redis-server-2
+ run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2"
+ continue-on-error: true🧰 Tools
🪛 yamllint (1.35.1)
[error] 68-68: trailing spaces
(trailing-spaces)
.github/workflows/test.yml
Outdated
| - name: wait for nodes to be ready | ||
| run: sleep 90 |
There was a problem hiding this comment.
Replace fixed sleep with proper health checks
Using a fixed sleep duration is not reliable. Replace this with the health checks suggested above.
Remove these lines after implementing the health checks above.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
35-45: Optimize host file modificationsThe current implementation makes multiple calls to sudo. Consider optimizing it:
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + cat << EOF | sudo tee -a /etc/hosts + 127.0.0.1 redis-standalone + 127.0.0.1 sentinel-1 + 127.0.0.1 sentinel-2 + 127.0.0.1 sentinel-3 + 127.0.0.1 redis-server-1 + 127.0.0.1 redis-server-2 + $(for i in {0..5}; do echo "127.0.0.1 node$i"; done) + EOF
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
82-82: property "platform" is not defined in object type {python: number}
(expression)
🔇 Additional comments (4)
.github/workflows/test.yml (4)
20-34: Add health checks and standardize Docker configuration
- Add health checks to ensure services are ready before proceeding:
- name: Run docker-compose for redis standalone instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/standalone-compose.yml + - name: Wait for standalone to be ready + run: | + timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done' - name: Run docker-compose for redis replication instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/sentinel-compose.yml + - name: Wait for sentinel to be ready + run: | + timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done' - name: Run docker-compose for redis cluster instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/cluster-compose.yml + - name: Wait for cluster to be ready + run: | + timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'
- Let's verify if the compose files are using the standard
FalkorDB/FalkorDBimage as requested.#!/bin/bash # Check if compose files are using the standard FalkorDB image for file in docker/*compose.yml; do echo "Checking $file for FalkorDB image..." grep -A 2 "image:" "$file" done🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
60-61: Move docker dependency to project dependenciesInstead of installing the docker module at runtime, add it to your project's dependencies in pyproject.toml.
- - name: install docker module for python - run: poetry add dockerAdd to your pyproject.toml:
[tool.poetry.dependencies] docker = "^6.1.3"
66-76: Add error handling for log collectionThe log retrieval steps should handle cases where containers might not exist or logs are not available.
- name: Get logs from node1 - run: docker logs node0 + run: docker logs node0 || echo "Failed to retrieve logs from node0" + continue-on-error: true - name: Get logs from node1 - run: docker logs node1 + run: docker logs node1 || echo "Failed to retrieve logs from node1" + continue-on-error: true - name: Get logs from node5 - run: docker logs node5 + run: docker logs node5 || echo "Failed to retrieve logs from node5" + continue-on-error: true - name: Get logs from redis-server-2 - run: docker logs redis-server-2 + run: docker logs redis-server-2 || echo "Failed to retrieve logs from redis-server-2" + continue-on-error: true🧰 Tools
🪛 yamllint (1.35.1)
[error] 68-68: trailing spaces
(trailing-spaces)
82-82: Fix undefined matrix variableThe
platformvariable is referenced but not defined in the matrix configuration.- name: Upload coverage uses: codecov/codecov-action@v5 - if: matrix.python == '3.10' && matrix.platform != 'macos-11' + if: matrix.python == '3.10' with: fail_ci_if_error: false token: ${{ secrets.CODECOV_TOKEN }}🧰 Tools
🪛 actionlint (1.7.4)
82-82: property "platform" is not defined in object type {python: number}
(expression)
.github/workflows/test.yml
Outdated
| - name: wait for nodes to connect to each other | ||
| run: while true;do (docker logs node0 | grep succeeded ) && ( docker logs node1 | grep succeeded ) && break ;done |
There was a problem hiding this comment.
Replace infinite loop with proper health check
The current implementation could hang indefinitely. Replace with a timeout-based health check:
- - name: wait for nodes to connect to each other
- run: while true;do (docker logs node0 | grep succeeded ) && ( docker logs node1 | grep succeeded ) && break ;done
+ - name: Wait for cluster nodes to connect
+ run: |
+ timeout 60s bash -c '
+ until docker logs node0 2>&1 | grep -q "succeeded" && \
+ docker logs node1 2>&1 | grep -q "succeeded"; do
+ echo "Waiting for nodes to connect..."
+ sleep 2
+ done
+ '📝 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.
| - name: wait for nodes to connect to each other | |
| run: while true;do (docker logs node0 | grep succeeded ) && ( docker logs node1 | grep succeeded ) && break ;done | |
| - name: Wait for cluster nodes to connect | |
| run: | | |
| timeout 60s bash -c ' | |
| until docker logs node0 2>&1 | grep -q "succeeded" && \ | |
| docker logs node1 2>&1 | grep -q "succeeded"; do | |
| echo "Waiting for nodes to connect..." | |
| sleep 2 | |
| done | |
| ' |
MuhammadQadora
force-pushed
the
71-add-support-to-get-replica-connections
branch
from
a481a26 to
af4f748
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
tests/test_get_replica_connections.py (2)
1-5: Remove unused import.The
subprocessmodule is imported but never used.import time import pytest -import subprocess from falkordb import FalkorDB import docker🧰 Tools
🪛 Ruff (0.8.2)
3-3:
subprocessimported but unusedRemove unused import:
subprocess(F401)
31-38: Improve client initialization with a factory pattern.The client initialization functions have duplicated logic and hardcoded parameters.
from typing import Callable from dataclasses import dataclass, field @dataclass class ClientConfig: host: str = 'localhost' ports: dict[str, int] = field(default_factory=lambda: { 'cluster': CLUSTER_PORT, 'standalone': STANDALONE_PORT, 'sentinel': SENTINEL_PORT }) def create_client(mode: str) -> Callable[[], FalkorDB]: """Create a FalkorDB client factory function. Args: mode: One of 'cluster', 'standalone', or 'sentinel' """ config = ClientConfig() def client_factory() -> FalkorDB: return FalkorDB(host=config.host, port=config.ports[mode]) return client_factory cluster_client = create_client('cluster') standalone_client = create_client('standalone') sentinel_client = create_client('sentinel')docker/cluster-compose.yml (1)
124-124: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.yml(3 hunks)docker/cluster-compose.yml(1 hunks)docker/sentinel-compose.yml(1 hunks)tests/test_get_replica_connections.py(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
docker/cluster-compose.yml
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 124-124: no new line character at the end of file
(new-line-at-end-of-file)
docker/sentinel-compose.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[warning] 74-74: too many spaces after colon
(colons)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
[warning] 87-87: too many spaces after colon
(colons)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
71-71: property "platform" is not defined in object type {python: number}
(expression)
🪛 Ruff (0.8.2)
tests/test_get_replica_connections.py
3-3: subprocess imported but unused
Remove unused import: subprocess
(F401)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (6)
.github/workflows/test.yml (4)
35-46: LGTM! Host file modifications are well implemented.The host file modifications are correctly implemented with proper sudo usage and efficient looping for node entries.
71-74:⚠️ Potential issueFix undefined matrix variable.
The
platformvariable is referenced but not defined in the matrix configuration.- name: Upload coverage uses: codecov/codecov-action@v5 - if: matrix.python == '3.10' && matrix.platform != 'macos-11' + if: matrix.python == '3.10' with: fail_ci_if_error: false token: ${{ secrets.CODECOV_TOKEN }}Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
71-71: property "platform" is not defined in object type {python: number}
(expression)
63-64:⚠️ Potential issueReplace infinite loop with proper timeout mechanism.
The current implementation could hang indefinitely. Use a timeout-based approach with proper error handling.
- - name: wait for nodes to connect to each other - run: while true;do (docker logs node0 | grep succeeded ) && ( docker logs node1 | grep succeeded ) && break ;done + - name: Wait for cluster nodes to connect + run: | + timeout 60s bash -c ' + until docker logs node0 2>&1 | grep -q "succeeded" && \ + docker logs node1 2>&1 | grep -q "succeeded"; do + echo "Waiting for nodes to connect..." + sleep 2 + done + 'Likely invalid or redundant comment.
20-34: 🛠️ Refactor suggestionAdd health checks before proceeding with tests.
The workflow should wait for each Redis service to be healthy before proceeding with tests to prevent race conditions.
Add health checks after each compose step:
- name: Run docker-compose for redis standalone instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/standalone-compose.yml + - name: Wait for standalone to be ready + run: | + timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done' - name: Run docker-compose for redis replication instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/sentinel-compose.yml + - name: Wait for sentinel to be ready + run: | + timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done' - name: Run docker-compose for redis cluster instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/cluster-compose.yml + - name: Wait for cluster to be ready + run: | + timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
docker/sentinel-compose.yml (1)
90-91: LGTM! Network configuration is appropriate.The network configuration is correctly defined for the Redis sentinel setup.
docker/cluster-compose.yml (1)
36-48: Same configuration improvements needed for node2The same improvements suggested for node1 should be applied here.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 48-48: trailing spaces
(trailing-spaces)
| - name: install docker module for python | ||
| run: poetry add docker |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move docker dependency to project dependencies.
Installing dependencies at runtime is not recommended. Add the docker package to your project's dependencies.
Remove these lines and add to your pyproject.toml:
[tool.poetry.dependencies]
docker = "^6.1.3"| def stop_replicas(client): | ||
| result = client.get_replica_connections() | ||
| for i in result: | ||
| name = i[0] | ||
| stop_container(name) | ||
|
|
||
| def start_replicas(replicas: list): | ||
| for replica in replicas: | ||
| start_container(replica) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Redis commands instead of container management.
As suggested by reviewer swilly22, use SLAVEOF NOONE command instead of stopping containers.
def stop_replicas(client: FalkorDB) -> None:
"""Disconnect replicas using SLAVEOF NOONE command."""
for name, port in client.get_replica_connections():
try:
replica = FalkorDB(host='localhost', port=port)
replica.execute_command('SLAVEOF', 'NOONE')
except Exception as e:
raise RuntimeError(f"Failed to disconnect replica {name}: {e}")
def start_replicas(replicas: list[tuple[str, int]]) -> None:
"""Reconnect replicas to their master."""
for name, port in replicas:
try:
replica = FalkorDB(host='localhost', port=port)
replica.execute_command('SLAVEOF', 'localhost', '6379')
except Exception as e:
raise RuntimeError(f"Failed to reconnect replica {name}: {e}")| def test_get_replica_connections_cluster_no_replicas(): | ||
| # assume this cluster has no replicas configured | ||
| stop_replicas(cluster_client()) | ||
| time.sleep(2) | ||
| c = cluster_client() | ||
| assert c.get_replica_connections() == [] | ||
| start_replicas(["node3", "node4", "node5"]) | ||
|
|
||
|
|
||
|
|
||
| def test_get_replica_connections_sentinel_no_replicas(): | ||
| # Assume this Sentinel setup has no replicas | ||
| stop_replicas(sentinel_client()) | ||
| time.sleep(2) | ||
| c = sentinel_client() | ||
| assert c.get_replica_connections() == [] | ||
| start_replicas(["redis-server-2"]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sleep times with health checks and add cleanup.
The tests use hardcoded sleep times and lack proper cleanup mechanisms.
from contextlib import contextmanager
def wait_for_health(client: FalkorDB, timeout: int = 30) -> None:
"""Wait for service to be healthy with exponential backoff."""
start = time.time()
delay = 1
while time.time() - start < timeout:
try:
client.ping()
return
except Exception:
time.sleep(delay)
delay = min(delay * 2, 5)
raise TimeoutError("Service did not become healthy")
@contextmanager
def temporary_no_replicas(mode: str):
"""Context manager to temporarily remove replicas and restore them."""
client = globals()[f"{mode}_client"]()
try:
stop_replicas(client)
wait_for_health(client)
yield
finally:
start_replicas(client.get_replica_connections())
wait_for_health(client)
def test_get_replica_connections_cluster_no_replicas():
with temporary_no_replicas('cluster'):
c = cluster_client()
assert c.get_replica_connections() == []
def test_get_replica_connections_sentinel_no_replicas():
with temporary_no_replicas('sentinel'):
c = sentinel_client()
assert c.get_replica_connections() == []| def docker_client(): | ||
| client = docker.from_env() | ||
| return client | ||
| # check=True is used to raise an exception if the command fails | ||
|
|
||
| def stop_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).stop() | ||
|
|
||
| def start_container(container_name): | ||
| client = docker_client() | ||
| client.containers.get(container_name).start() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Docker client functions with proper error handling and type hints.
The Docker client functions lack error handling, type hints, and documentation.
-def docker_client():
- client = docker.from_env()
- return client
+def docker_client() -> docker.DockerClient:
+ """Create and return a Docker client.
+
+ Returns:
+ docker.DockerClient: Initialized Docker client
+
+ Raises:
+ RuntimeError: If connection to Docker daemon fails
+ """
+ try:
+ return docker.from_env()
+ except docker.errors.DockerException as e:
+ raise RuntimeError(f"Failed to connect to Docker daemon: {e}")
-def stop_container(container_name):
- client = docker_client()
- client.containers.get(container_name).stop()
+def stop_container(container_name: str) -> None:
+ """Stop a Docker container.
+
+ Args:
+ container_name: Name of the container to stop
+
+ Raises:
+ RuntimeError: If container not found or stop operation fails
+ """
+ try:
+ client = docker_client()
+ container = client.containers.get(container_name)
+ container.stop(timeout=10)
+ except docker.errors.NotFound:
+ raise RuntimeError(f"Container {container_name} not found")
+ except Exception as e:
+ raise RuntimeError(f"Failed to stop container {container_name}: {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.
| def docker_client(): | |
| client = docker.from_env() | |
| return client | |
| # check=True is used to raise an exception if the command fails | |
| def stop_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).stop() | |
| def start_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).start() | |
| def docker_client() -> docker.DockerClient: | |
| """Create and return a Docker client. | |
| Returns: | |
| docker.DockerClient: Initialized Docker client | |
| Raises: | |
| RuntimeError: If connection to Docker daemon fails | |
| """ | |
| try: | |
| return docker.from_env() | |
| except docker.errors.DockerException as e: | |
| raise RuntimeError(f"Failed to connect to Docker daemon: {e}") | |
| def stop_container(container_name: str) -> None: | |
| """Stop a Docker container. | |
| Args: | |
| container_name: Name of the container to stop | |
| Raises: | |
| RuntimeError: If container not found or stop operation fails | |
| """ | |
| try: | |
| client = docker_client() | |
| container = client.containers.get(container_name) | |
| container.stop(timeout=10) | |
| except docker.errors.NotFound: | |
| raise RuntimeError(f"Container {container_name} not found") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to stop container {container_name}: {e}") | |
| def start_container(container_name): | |
| client = docker_client() | |
| client.containers.get(container_name).start() |
| services: | ||
| sentinel-1: | ||
| image: bitnami/redis-sentinel | ||
| container_name: sentinel-1 | ||
| hostname: sentinel-1 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26379:26379 | ||
| environment: | ||
| REDIS_MASTER_SET: master | ||
| REDIS_MASTER_HOST: redis-server-1 | ||
| REDIS_MASTER_PORT_NUMBER: 6380 | ||
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-1 | ||
| REDIS_SENTINEL_PORT_NUMBER: 26379 | ||
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | ||
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | ||
| ALLOW_EMPTY_PASSWORD: "yes" | ||
|
|
||
| sentinel-2: | ||
| image: bitnami/redis-sentinel | ||
| container_name: sentinel-2 | ||
| hostname: sentinel-2 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26380:26380 | ||
| environment: | ||
| REDIS_MASTER_SET: master | ||
| REDIS_MASTER_HOST: redis-server-1 | ||
| REDIS_MASTER_PORT_NUMBER: 6380 | ||
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-2 | ||
| REDIS_SENTINEL_PORT_NUMBER: 26380 | ||
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | ||
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | ||
| ALLOW_EMPTY_PASSWORD: "yes" | ||
|
|
||
| sentinel-3: | ||
| image: bitnami/redis-sentinel | ||
| container_name: sentinel-3 | ||
| hostname: sentinel-3 | ||
| restart: always | ||
| networks: | ||
| - redis-sentinel | ||
| ports: | ||
| - 26381:26381 | ||
| environment: | ||
| REDIS_MASTER_SET: master | ||
| REDIS_MASTER_HOST: redis-server-1 | ||
| REDIS_MASTER_PORT_NUMBER: 6380 | ||
| REDIS_SENTINEL_ANNOUNCE_PORT: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_HOSTNAMES: "yes" | ||
| REDIS_SENTINEL_ANNOUNCE_IP: sentinel-3 | ||
| REDIS_SENTINEL_PORT_NUMBER: 26381 | ||
| REDIS_SENTINEL_DOWN_AFTER_MILLISECONDS: 500 | ||
| REDIS_SENTINEL_FAILOVER_TIMEOUT: 500 | ||
| ALLOW_EMPTY_PASSWORD: "yes" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health checks and improve sentinel configuration.
The sentinel services lack health checks and have duplicated configuration.
sentinel-1:
image: bitnami/redis-sentinel
container_name: sentinel-1
hostname: sentinel-1
restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "26379", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
networks:
- redis-sentinelApply similar changes to sentinel-2 and sentinel-3. Consider using YAML anchors to reduce configuration duplication:
x-sentinel-base: &sentinel-base
image: bitnami/redis-sentinel
restart: always
networks:
- redis-sentinel
healthcheck: &healthcheck
test: ["CMD", "redis-cli", "-p", "26379", "ping"]
interval: 5s
timeout: 3s
retries: 3🧰 Tools
🪛 yamllint (1.35.1)
[error] 43-43: trailing spaces
(trailing-spaces)
| redis-server-2: | ||
| image: falkordb/falkordb:latest | ||
| hostname: redis-server-2 | ||
| container_name: redis-server-2 | ||
| ports: | ||
| - 6381:6381 | ||
| networks: | ||
| - redis-sentinel | ||
| environment: | ||
| REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-2 --replica-announce-port 6381 --replicaof redis-server-1 6380" | ||
| BROWSER: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Redis replica configuration and add master dependency.
The Redis replica needs similar improvements as the master and should wait for master to be healthy.
redis-server-2:
image: falkordb/falkordb:latest
hostname: redis-server-2
container_name: redis-server-2
+ restart: always
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6381", "ping"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
+ depends_on:
+ redis-server-1:
+ condition: service_healthy
+ volumes:
+ - redis_replica_data:/data
ports:
- 6381:6381
networks:
- redis-sentinel
environment:
- REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-2 --replica-announce-port 6381 --replicaof redis-server-1 6380"
+ REDIS_ARGS: "--port 6381 --replica-announce-ip redis-server-2 --replica-announce-port 6381 --replicaof redis-server-1 6380"Add to volumes section:
redis_replica_data:🧰 Tools
🪛 yamllint (1.35.1)
[warning] 87-87: too many spaces after colon
(colons)
| node0: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node0 | ||
| hostname: node0 | ||
| ports: | ||
| - 5000:5000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node0-data:/data | ||
| healthcheck: | ||
| test: | | ||
| redis-cli --cluster create node0:5000 node1:6000 \ | ||
| node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 1s | ||
| environment: | ||
| REDIS_ARGS: "--port 5000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node0" | ||
|
|
There was a problem hiding this comment.
Critical issues in node0 configuration
Several critical issues need to be addressed:
- Cluster creation should not be in health check
- Missing restart policy
- Configuration through environment variables is less maintainable
node0:
image: falkordb/falkordb:latest
container_name: node0
hostname: node0
+ restart: unless-stopped
ports:
- 5000:5000
networks:
- redis-cluster
volumes:
- node0-data:/data
+ - ./config/node0.conf:/data/node.conf:ro
healthcheck:
- test: |
- redis-cli --cluster create node0:5000 node1:6000 \
- node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes || echo failed && exit 1
- interval: 3s
- timeout: 10s
- retries: 10
- start_period: 1s
+ test: ["CMD", "redis-cli", "-p", "5000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
- environment:
- REDIS_ARGS: "--port 5000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node0"
+ command: redis-server /data/node.confCreate a configuration file config/node0.conf:
port 5000
loadmodule /FalkorDB/bin/src/falkordb.so
cluster-enabled yes
cluster-port 16379
cluster-preferred-endpoint-type hostname
cluster-config-file /data/nodes.conf
cluster-node-timeout 500
cluster-announce-hostname node0
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 10
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
| healthcheck: | ||
| test: | | ||
| #!/bin/bash | ||
| ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n') | ||
| redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1 | ||
| interval: 3s | ||
| timeout: 10s | ||
| retries: 10 | ||
| start_period: 30s |
There was a problem hiding this comment.
Fix health check logic in replica nodes
The health checks in nodes 3-5 have several issues:
- Complex shell scripts in health checks
- Incorrect shell variable syntax (
$$ipinstead of$ip) - The
|| echo failed && exit 1logic is incorrect (will always exit)
healthcheck:
- test: |
- #!/bin/bash
- ip=$(getent hosts node2 |awk '{print $1}'| tr -d '\r\n')
- redis-cli -p 8000 cluster meet $$ip 7000 || echo failed && exit 1
+ test: ["CMD", "redis-cli", "-p", "8000", "cluster", "info"]
interval: 5s
timeout: 3s
- retries: 10
- start_period: 30s
+ retries: 3Also applies to: 81-89, 103-111
| @@ -0,0 +1,124 @@ | |||
| services: | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dedicated cluster initialization service
A separate service should handle cluster initialization after all nodes are healthy.
cluster-init:
image: falkordb/falkordb:latest
command: >
/bin/bash -c "
echo 'Waiting for nodes to be ready...' &&
sleep 10 &&
redis-cli --cluster create node0:5000 node1:6000 node2:7000 node3:8000 node4:9000 node5:10000 --cluster-replicas 1 --cluster-yes
"
networks:
- redis-cluster
depends_on:
node0:
condition: service_healthy
node1:
condition: service_healthy
node2:
condition: service_healthy
node3:
condition: service_healthy
node4:
condition: service_healthy
node5:
condition: service_healthy| node1: | ||
| image: falkordb/falkordb:latest | ||
| container_name: node1 | ||
| hostname: node1 | ||
| ports: | ||
| - 6000:6000 | ||
| networks: | ||
| - redis-cluster | ||
| volumes: | ||
| - node1-data:/data | ||
| environment: | ||
| REDIS_ARGS: "--port 6000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node1" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health check and improve configuration for node1
Similar to node0, this node needs:
- A restart policy
- Health check configuration
- Configuration file instead of environment variables
node1:
image: falkordb/falkordb:latest
container_name: node1
hostname: node1
+ restart: unless-stopped
ports:
- 6000:6000
networks:
- redis-cluster
volumes:
- node1-data:/data
+ - ./config/node1.conf:/data/node.conf:ro
+ healthcheck:
+ test: ["CMD", "redis-cli", "-p", "6000", "cluster", "info"]
+ interval: 5s
+ timeout: 3s
+ retries: 3
- environment:
- REDIS_ARGS: "--port 6000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node1"
+ command: redis-server /data/node.conf📝 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.
| node1: | |
| image: falkordb/falkordb:latest | |
| container_name: node1 | |
| hostname: node1 | |
| ports: | |
| - 6000:6000 | |
| networks: | |
| - redis-cluster | |
| volumes: | |
| - node1-data:/data | |
| environment: | |
| REDIS_ARGS: "--port 6000 --cluster-enabled yes --cluster-port 16379 --cluster-preferred-endpoint-type hostname --cluster-config-file /data/nodes.conf --cluster-node-timeout 500 --cluster-announce-hostname node1" | |
| node1: | |
| image: falkordb/falkordb:latest | |
| container_name: node1 | |
| hostname: node1 | |
| restart: unless-stopped | |
| ports: | |
| - 6000:6000 | |
| networks: | |
| - redis-cluster | |
| volumes: | |
| - node1-data:/data | |
| - ./config/node1.conf:/data/node.conf:ro | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "6000", "cluster", "info"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 3 | |
| command: redis-server /data/node.conf |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 35-35: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/test.yml (2)
20-34: 🛠️ Refactor suggestionAdd health checks for Redis services
The workflow should wait for each Redis service to be healthy before proceeding with tests.
Apply this diff:
- name: Run docker-compose for redis standalone instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/standalone-compose.yml + - name: Wait for standalone to be ready + run: | + timeout 30s bash -c 'until docker exec redis-standalone redis-cli ping; do sleep 1; done' - name: Run docker-compose for redis replication instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/sentinel-compose.yml + - name: Wait for sentinel to be ready + run: | + timeout 30s bash -c 'until docker exec sentinel-1 redis-cli -p 26379 ping; do sleep 1; done' - name: Run docker-compose for redis cluster instance uses: hoverkraft-tech/[email protected] with: compose-file: ./docker/cluster-compose.yml + - name: Wait for cluster to be ready + run: | + timeout 30s bash -c 'until docker exec node0 redis-cli -p 5000 cluster info; do sleep 1; done'🧰 Tools
🪛 yamllint (1.35.1)
[error] 34-34: trailing spaces
(trailing-spaces)
60-61: 🛠️ Refactor suggestionMove docker dependency to project dependencies
Installing dependencies at runtime is not recommended. Add the docker package to your project's dependencies.
Remove these lines and add to your pyproject.toml:
[tool.poetry.dependencies] docker = "^6.1.3"
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
35-45: Optimize host file modificationsThe current implementation uses redundant sudo commands. The script can be more concise.
- name: Add entries to the host file to correctly resolve service names run: | - sudo echo "127.0.0.1 redis-standalone" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-2" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 sentinel-3" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-1" | sudo tee -a /etc/hosts - sudo echo "127.0.0.1 redis-server-2" | sudo tee -a /etc/hosts - for i in node{0..5};do - echo "127.0.0.1 $i" | sudo tee -a /etc/hosts - done + { + echo "127.0.0.1 redis-standalone" + echo "127.0.0.1 sentinel-1" + echo "127.0.0.1 sentinel-2" + echo "127.0.0.1 sentinel-3" + echo "127.0.0.1 redis-server-1" + echo "127.0.0.1 redis-server-2" + for i in {0..5}; do echo "127.0.0.1 node$i"; done + } | sudo tee -a /etc/hosts
11-11: Remove trailing spacesThere are trailing spaces on lines 11, 34, 59, and 68.
Also applies to: 34-34, 59-59, 68-68
🧰 Tools
🪛 yamllint (1.35.1)
[error] 11-11: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
71-71: property "platform" is not defined in object type {python: number}
(expression)
🔇 Additional comments (2)
.github/workflows/test.yml (2)
71-71:⚠️ Potential issueRemove undefined matrix variable condition
The
matrix.platformvariable is referenced but not defined in the matrix configuration.- if: matrix.python == '3.10' && matrix.platform != 'macos-11' + if: matrix.python == '3.10'Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
71-71: property "platform" is not defined in object type {python: number}
(expression)
63-64: 🛠️ Refactor suggestionReplace infinite loop with proper health check
The current implementation could hang indefinitely. Replace with a timeout-based health check.
- - name: wait for nodes to connect to each other - run: while true;do (docker logs node0 | grep succeeded ) && ( docker logs node1 | grep succeeded ) && ( docker logs node2 | grep succeeded ) && break ;done + - name: Wait for cluster nodes to connect + run: | + timeout 60s bash -c ' + until docker logs node0 2>&1 | grep -q "succeeded" && \ + docker logs node1 2>&1 | grep -q "succeeded" && \ + docker logs node2 2>&1 | grep -q "succeeded"; do + echo "Waiting for nodes to connect..." + sleep 2 + done + 'Likely invalid or redundant comment.
fix #71
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation