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 all 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
49 changes: 37 additions & 12 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,13 +56,19 @@ jobs:

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

- name: install docker module for python
run: poetry add docker
Comment on lines +60 to +61
Copy link

Choose a reason for hiding this comment

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

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


- 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: Run Tests
run: poetry run pytest --cov --cov-report=xml

- name: Upload coverage
uses: codecov/codecov-action@v5
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)

124 changes: 124 additions & 0 deletions docker/cluster-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
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:latest
container_name: node0
hostname: node0
ports:
- 5000:5000
networks:
- redis-cluster
volumes:
- node0-data:/data
Comment on lines +3 to +11
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 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.

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

Comment on lines +2 to +11
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 restart policy and improve configuration management

The node configuration is missing important stability features:

  1. No restart policy defined
  2. 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:/data

Create 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

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"

Comment on lines +1 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical issues in node0 configuration

Several critical issues need to be addressed:

  1. Cluster creation should not be in health check
  2. Missing restart policy
  3. 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.conf

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

Comment on lines +2 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical issues in node0 configuration

Several critical issues need to be addressed:

  1. Cluster creation should not be in health check
  2. Missing restart policy
  3. 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.conf

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

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"

Comment on lines +23 to +35
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 check and improve configuration for node1

Similar to node0, this node needs:

  1. A restart policy
  2. Health check configuration
  3. 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.

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

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: 30s
Comment on lines +59 to +67
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 in replica nodes

The health checks in nodes 3-5 have several issues:

  1. Complex shell scripts in health checks
  2. Incorrect shell variable syntax ($$ip instead of $ip)
  3. The || echo failed && exit 1 logic 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: 3

Also applies to: 81-89, 103-111

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: 30s
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: 30s
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:
91 changes: 91 additions & 0 deletions docker/sentinel-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
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"

Comment on lines +2 to +22
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 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.

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

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"

Comment on lines +1 to +64
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 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-sentinel

Apply 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-1:
image: falkordb/falkordb:latest
hostname: redis-server-1
container_name: redis-server-1
ports:
- 6380:6380
networks:
- redis-sentinel
environment:
REDIS_ARGS: "--port 6380 --replica-announce-ip redis-server-1 --replica-announce-port 6380"
BROWSER: 0

Comment on lines +65 to +76
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 configuration for production readiness.

The Redis master configuration lacks several important production features.

   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
     ports:
       - 6380:6380
     networks:
       - redis-sentinel
     environment:
-      REDIS_ARGS:  "--port 6380 --replica-announce-ip redis-server-1 --replica-announce-port 6380"
+      REDIS_ARGS: "--port 6380 --replica-announce-ip redis-server-1 --replica-announce-port 6380"

Add at the end of the file:

volumes:
  redis_master_data:
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 74-74: too many spaces after colon

(colons)


[error] 76-76: 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
Comment on lines +78 to +88
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 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)


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:latest
ports:
- 6379:6379
34 changes: 34 additions & 0 deletions falkordb/falkordb.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,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:
return replica_hostnames
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

elif Is_Cluster(self.connection):
try:
data = self.connection.get_replicas()
if not data:
return data
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