Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added get_replica_connections function #72

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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,7 @@ jobs:
test:
name: Test with Python ${{ matrix.python }}
runs-on: ubuntu-latest

services:
falkordb:
# Docker Hub image
image: falkordb/falkordb:edge
# Map port 6379 on the Docker host to port 6379 on the FalkorDB container
ports:
- 6379:6379


strategy:
matrix:
python: ['3.8', '3.10', '3.11']
Expand All @@ -25,9 +17,36 @@ jobs:
steps:
- uses: actions/checkout@v4

- name: Run docker-compose for redis standalone instance
uses: hoverkraft-tech/[email protected]
with:
compose-file: ./docker/standalone-compose.yml
Comment on lines +20 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
- 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


- 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

Comment on lines +20 to +34
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
- 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)

- 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

- uses: actions/setup-python@v5
with:
python-version: ${{matrix.python}}
python-version: ${{ matrix.python }}

- uses: snok/install-poetry@v1
with:
Expand All @@ -37,6 +56,9 @@ jobs:

- name: Install dependencies
run: poetry install --no-interaction

- name: install docker-compose client
run: sudo apt update && sudo apt install docker-compose -y

- name: Run Tests
run: poetry run pytest --cov --cov-report=xml
Expand All @@ -46,4 +68,4 @@ jobs:
if: matrix.python == '3.10' && matrix.platform != 'macos-11'
with:
fail_ci_if_error: false
token: ${{ secrets.CODECOV_TOKEN }}
token: ${{ secrets.CODECOV_TOKEN }}
Comment on lines 71 to +74
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)

134 changes: 134 additions & 0 deletions docker/cluster-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
services:
Copy link

Choose a reason for hiding this comment

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

🛠️ 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

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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix health check logic and add init container for cluster creation

The current health check has several issues:

  1. The cluster creation command is not appropriate for health checking
  2. The || echo failed && exit 1 logic is incorrect (will always exit)
  3. 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_successfully

Add 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)

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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix cluster initialization and health check logic

Critical issues in node0 configuration:

  1. Cluster creation in healthcheck is incorrect
  2. Missing restart policy
  3. 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: 3

Add 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)

node1:
image: falkordb/falkordb:edge
container_name: node1
hostname: node1
ports:
- 6000:6000
networks:
- redis-cluster
entrypoint: ["/bin/bash", "-c"]
command: |
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", "-c"]
command: |
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", "-c"]
command: |
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", "-c"]
command: |
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", "-c"]
command: |
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


networks:
redis-cluster:
98 changes: 98 additions & 0 deletions docker/sentinel-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
services:
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
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add health checks and improve reliability for sentinel services

The sentinel services need improvements:

  1. Missing health checks
  2. Inconsistent restart policies
  3. 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:26379

Apply 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)


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

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve Redis master server configuration

The Redis master server configuration needs improvements:

  1. Missing health check
  2. Missing restart policy
  3. 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-sentinel

Add 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)

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
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve Redis replica server configuration

The Redis replica server has similar issues:

  1. Missing health check
  2. Missing restart policy
  3. 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_healthy

Add to volumes section:

  redis_replica_data:
🧰 Tools
🪛 yamllint (1.35.1)

[error] 90-90: trailing spaces

(trailing-spaces)


networks:
redis-sentinel:
7 changes: 7 additions & 0 deletions docker/standalone-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
services:
redis-standalone:
container_name: redis-standalone
hostname: redis-standalone
image: falkordb/falkordb:edge
Copy link

Choose a reason for hiding this comment

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

🛠️ 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:latest

Committable suggestion skipped: line range outside the PR's diff.

ports:
- 6379:6379
37 changes: 37 additions & 0 deletions falkordb/falkordb.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,13 @@ def from_url(cls, url: str, **kwargs) -> "FalkorDB":
conn = redis.from_url(url, **kwargs)

connection_kwargs = conn.connection_pool.connection_kwargs
connection_class = conn.connection_pool.connection_class
kwargs["host"] = connection_kwargs.get("host", "localhost")
kwargs["port"] = connection_kwargs.get("port", 6379)
kwargs["username"] = connection_kwargs.get("username")
kwargs["password"] = connection_kwargs.get("password")
if connection_class is redis.SSLConnection:
kwargs["ssl"] = True

# Initialize a FalkorDB instance using the updated kwargs
db = cls(**kwargs)
Expand Down Expand Up @@ -224,7 +227,41 @@ def config_get(self, name: str) -> Union[int, str]:
"""

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

def get_replica_connections(self):
"""
Retrieve a list of connections for FalkorDB replicas.

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.
"""
if hasattr(self, 'sentinel') and self.sentinel is not None:
try:
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)

if not replica_hostnames:
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.") from e
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update deprecated Redis terminology and improve error handling

  1. Replace discover_slaves with discover_replicas to align with current Redis terminology
  2. Make error messages more specific

Apply this diff:

 try:
-    replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
+    replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)
     if not replica_hostnames:
         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.") from e
+    raise ConnectionError(f"Failed to discover replicas in Sentinel mode: {str(e)}") 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.

Suggested change
if hasattr(self, 'sentinel') and self.sentinel is not None:
try:
replica_hostnames = self.sentinel.discover_slaves(service_name=self.service_name)
if not replica_hostnames:
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.") from e
if hasattr(self, 'sentinel') and self.sentinel is not None:
try:
replica_hostnames = self.sentinel.discover_replicas(service_name=self.service_name)
if not replica_hostnames:
raise ConnectionError("Unable to get replica hostname.")
return [(host, int(port)) for host, port in replica_hostnames]
except redis.RedisError as e:
raise ConnectionError(f"Failed to discover replicas in Sentinel mode: {str(e)}") from e


elif Is_Cluster(self.connection):
try:
data = self.connection.get_replicas()
if not data:
raise ConnectionError("Unable to get cluster nodes")
return [ (i.host, i.port) for i in data]
except redis.RedisError as e:
raise ConnectionError("Failed to get replica hostnames") from e

else:
raise ValueError(f"Unsupported Redis mode")

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