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

Changed instructions on how to setup a FalkorDB Cluster #110

Merged

Conversation

MuhammadQadora
Copy link
Contributor

@MuhammadQadora MuhammadQadora commented Jan 5, 2025

fix #109

Summary by CodeRabbit

  • Documentation
    • Updated cluster setup instructions for FalkorDB using Docker.
    • Enhanced documentation for dynamic node creation, hostname resolution, and cluster configuration.
    • Improved guidance for executing graph queries and testing deployments.
    • Clarified steps for scaling the cluster and checking cluster status.
    • Added new entry hostnames to the .wordlist.txt file.

@MuhammadQadora MuhammadQadora linked an issue Jan 5, 2025 that may be closed by this pull request
Copy link
Contributor

coderabbitai bot commented Jan 5, 2025

Walkthrough

The pull request updates the documentation for setting up a FalkorDB cluster using Docker. The new instructions simplify the node creation process by introducing a loop to dynamically create six nodes with unique names and hostnames. The documentation now includes detailed steps for editing the /etc/hosts file, configuring node connections, and establishing the cluster. The changes provide more comprehensive guidance for cluster setup, including commands for creating and querying graphs, and steps for scaling the cluster.

Changes

File Change Summary
operations/cluster.md - Replaced manual node creation with a dynamic loop
- Added detailed instructions for /etc/hosts configuration
- Updated cluster connection and initialization commands
- Expanded graph creation and testing section
- Improved scaling process documentation
.wordlist.txt - Added new entry hostnames at the end of the file

Assessment against linked issues

Objective Addressed Explanation
Update documentation for FalkorDB cluster creation [#109]

Poem

🐰 In Docker's realm, a cluster takes flight,
Six nodes dancing, configuration just right!
Hostnames mapped, ports aligned with care,
FalkorDB's magic spreads everywhere!
A rabbit's guide to scaling with glee! 🚀


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ceaa80 and d4d7e98.

📒 Files selected for processing (1)
  • .wordlist.txt (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .wordlist.txt

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
operations/cluster.md (2)

37-46: LGTM! Consider adding a comment about port mapping.

The loop-based node creation is efficient and well-structured. Consider adding a comment explaining that ports 6379-6384 will be mapped to avoid confusion about the port calculation.

 for i in {1..6}; do
+  # Map ports 6379-6384 to corresponding container ports
   docker run -d \
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph


65-80: Fix section numbering inconsistency.

The section numbering jumps from 3.1 to 3.3, skipping 3.2.

Renumber the sections sequentially:

-### 3.3 Verify Cluster Status
+### 3.2 Verify Cluster Status
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9db97e and f19cbcf.

📒 Files selected for processing (1)
  • operations/cluster.md (2 hunks)
🧰 Additional context used
🪛 LanguageTool
operations/cluster.md

[grammar] ~82-~82: The plural noun “Graphs” cannot be used with the article “a”. Did you mean “a graph” or “Graphs”?
Context: ...roles (master/replica). ### 3.4 Create a Graphs to test deployment This will create a ...

(A_NNS)


[grammar] ~103-~103: The word “output” is spelled as one word.
Context: ...tring(id), age: 20 + id % 50})" ``` The out put will show that it was moved to the righ...

(OUT_COMPOUNDS)

🪛 Markdownlint (0.37.0)
operations/cluster.md

105-105: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Actions: spellcheck
operations/cluster.md

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph

🔇 Additional comments (1)
operations/cluster.md (1)

123-141: LGTM! Clear and accurate scaling instructions.

The instructions for adding a new node, including the hosts file update and cluster integration, are well-structured and technically accurate.

🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph

Comment on lines 82 to 110
### 3.4 Create a Graphs to test deployment

### 3.3 Verify Cluster Status
This will create a graph called testGraph and create nodes with different ids.

You can verify the status of the cluster with:
We can directly execute the command (does not show that the redis-cli moved):

```bash
redis-cli --cluster check node1:6379
redis-cli -c GRAPH.QUERY testGraph "UNWIND range(1, 100) AS id CREATE (n:Person {id: id, name: 'Person ' + toString(id), age: 20 + id % 50})"
```

Or we can run:
A):

```bash
redis-cli -c
```
B):

```bash
GRAPH.QUERY testGraph "UNWIND range(1, 100) AS id CREATE (n:Person {id: id, name: 'Person ' + toString(id), age: 20 + id % 50})"
```
The out put will show that it was moved to the right node:

```
-> Redirected to slot [15841] located at node3:6381
1) 1) "Nodes created: 100"
2) "Properties set: 300"
3) "Cached execution: 1"
4) "Query internal execution time: 0.505416 milliseconds"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix grammar and improve section structure.

Several issues need attention:

  1. Grammar error in heading: "Create a Graphs" should be "Create a Graph"
  2. The word "output" is incorrectly written as "out put"
  3. The code block at line 105 lacks a language specifier

Apply these corrections:

-### 3.4 Create a Graphs to test deployment
+### 3.4 Create a Graph to test deployment

-The out put will show that it was moved to the right node:
+The output will show that it was moved to the right node:

Add language specifier to the output code block:

-```
+```bash
 -> Redirected to slot [15841] located at node3:6381
🧰 Tools
🪛 LanguageTool

[grammar] ~82-~82: The plural noun “Graphs” cannot be used with the article “a”. Did you mean “a graph” or “Graphs”?
Context: ...roles (master/replica). ### 3.4 Create a Graphs to test deployment This will create a ...

(A_NNS)


[grammar] ~103-~103: The word “output” is spelled as one word.
Context: ...tring(id), age: 20 + id % 50})" ``` The out put will show that it was moved to the righ...

(OUT_COMPOUNDS)

🪛 Markdownlint (0.37.0)

105-105: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Actions: spellcheck

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph

Comment on lines 49 to 57
### 2.2 Edit the /etc/hosts fil and add the node container hostnames

```bash
docker run -d \
--name node3 \
--network falkordb-cluster-network \
-p 6381:6379 \
-e 'FALKORDB_ARGS=--cluster-enabled yes' \
falkordb/falkordb
```
For the Redis-cli to be able to connect and automoatically switch between nodes for an example when the (MOVED) operation happens,
we have to edit the /etc/hosts file to include the container hostnames.

```bash
docker run -d \
--name node4 \
--network falkordb-cluster-network \
-p 6382:6379 \
-e 'FALKORDB_ARGS=--cluster-enabled yes' \
falkordb/falkordb
for i in {1..6};do
sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts
done
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix spelling errors and improve error handling.

There are several issues in this section:

  1. Spelling errors:
    • "fil" should be "file"
    • "automoatically" should be "automatically"
  2. The command lacks error handling for sudo permissions

Apply these corrections:

-### 2.2 Edit the /etc/hosts fil and add the node container hostnames
+### 2.2 Edit the /etc/hosts file and add the node container hostnames

-For the Redis-cli to be able to connect and automoatically switch between nodes
+For the Redis-cli to be able to connect and automatically switch between nodes

Consider adding error handling:

 for i in {1..6};do
-  sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts
+  if ! sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts; then
+    echo "Error: Failed to update /etc/hosts for node$i. Please check sudo permissions."
+    exit 1
+  fi
 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
### 2.2 Edit the /etc/hosts fil and add the node container hostnames
```bash
docker run -d \
--name node3 \
--network falkordb-cluster-network \
-p 6381:6379 \
-e 'FALKORDB_ARGS=--cluster-enabled yes' \
falkordb/falkordb
```
For the Redis-cli to be able to connect and automoatically switch between nodes for an example when the (MOVED) operation happens,
we have to edit the /etc/hosts file to include the container hostnames.
```bash
docker run -d \
--name node4 \
--network falkordb-cluster-network \
-p 6382:6379 \
-e 'FALKORDB_ARGS=--cluster-enabled yes' \
falkordb/falkordb
for i in {1..6};do
sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts
done
### 2.2 Edit the /etc/hosts file and add the node container hostnames
For the Redis-cli to be able to connect and automatically switch between nodes for an example when the (MOVED) operation happens,
we have to edit the /etc/hosts file to include the container hostnames.
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error: 'fil' found in h3 section


[error] Spelling error: 'hostnames' found in h3 section and paragraph


[error] Spelling error: 'automoatically' found in paragraph


[error] Spelling error: 'testGraph' found in paragraph

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
operations/cluster.md (1)

70-70: Document --cluster-yes implications and expected output.

The --cluster-yes flag automatically accepts cluster creation without confirmation. This should be documented along with the expected successful output.

Add a note about the implications and expected output:

 docker exec -it node1 redis-cli --cluster create node1:6379 node2:6380 node3:6381 node4:6382 node5:6383 node6:6384 --cluster-replicas 1 --cluster-yes
+
+# Note: The --cluster-yes flag automatically accepts cluster creation without confirmation.
+# Expected successful output should show:
+# - Node assignments (master/replica)
+# - Hash slot distributions
+# - "OK" message indicating successful creation
🧰 Tools
🪛 GitHub Actions: spellcheck

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f19cbcf and d7d47bd.

📒 Files selected for processing (1)
  • operations/cluster.md (2 hunks)
🧰 Additional context used
🪛 LanguageTool
operations/cluster.md

[grammar] ~103-~103: The word “output” is spelled as one word.
Context: ...tring(id), age: 20 + id % 50})" ``` The out put will show that it was moved to the righ...

(OUT_COMPOUNDS)

🪛 Markdownlint (0.37.0)
operations/cluster.md

105-105: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Actions: spellcheck
operations/cluster.md

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'

🔇 Additional comments (2)
operations/cluster.md (2)

103-110: Fix grammar and code block formatting.

  1. "out put" should be "output"
  2. Add language specifier to the code block

Apply these corrections:

-The out put will show that it was moved to the right node:
+The output will show that it was moved to the right node:

-```
+```bash
 -> Redirected to slot [15841] located at node3:6381
 1) 1) "Nodes created: 100"
    2) "Properties set: 300"
    3) "Cached execution: 1"
    4) "Query internal execution time: 0.505416 milliseconds"
🧰 Tools
🪛 LanguageTool

[grammar] ~103-~103: The word “output” is spelled as one word.
Context: ...tring(id), age: 20 + id % 50})" ``` The out put will show that it was moved to the righ...

(OUT_COMPOUNDS)

🪛 Markdownlint (0.37.0)

105-105: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Actions: spellcheck

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'


55-57: ⚠️ Potential issue

Improve hosts file modification safety.

The current implementation modifies a system file without proper safeguards:

  1. No sudo permission check
  2. No backup of hosts file
  3. No validation of existing entries

Consider this safer implementation:

+# Backup hosts file
+sudo cp /etc/hosts /etc/hosts.bak_$(date +%Y%m%d_%H%M%S) || {
+  echo "Error: Failed to backup hosts file. Check sudo permissions."
+  exit 1
+}
+
 for i in {1..6};do
-  sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts
+  # Check if entry already exists
+  if ! grep -q "^127.0.0.1 node$i$" /etc/hosts; then
+    if ! sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts; then
+      echo "Error: Failed to update /etc/hosts. Check sudo permissions."
+      exit 1
+    fi
+  fi
 done

Likely invalid or redundant comment.

🧰 Tools
🪛 GitHub Actions: spellcheck

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'


```bash
docker exec -it node1 /bin/bash
redis-cli --cluster add-node node7:6379 node1:6379
docker exec -it node1 redis-cli --cluster add-node node7:6385 node1:6379
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add rebalancing instructions after node addition.

The scaling section should include critical information about rebalancing the cluster after adding a new node.

Add rebalancing instructions:

 docker exec -it node1 redis-cli --cluster add-node node7:6385 node1:6379
+
+# Rebalance the cluster to distribute slots to the new node
+docker exec -it node1 redis-cli --cluster rebalance node1:6379
+
+# Note: Rebalancing may impact performance. Consider running during off-peak hours.
+# Monitor the rebalancing progress and cluster health:
+docker exec -it node1 redis-cli --cluster check node1:6379
📝 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
docker exec -it node1 redis-cli --cluster add-node node7:6385 node1:6379
docker exec -it node1 redis-cli --cluster add-node node7:6385 node1:6379
# Rebalance the cluster to distribute slots to the new node
docker exec -it node1 redis-cli --cluster rebalance node1:6379
# Note: Rebalancing may impact performance. Consider running during off-peak hours.
# Monitor the rebalancing progress and cluster health:
docker exec -it node1 redis-cli --cluster check node1:6379
🧰 Tools
🪛 GitHub Actions: spellcheck

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'

Comment on lines +37 to +46
for i in {1..6}; do
docker run -d \
--name node$i \
--hostname node$i \
--network falkordb-cluster-network \
-p $((6379 + i - 1)):$((6379 + i - 1)) \
-e BROWSER=0 \
-e "FALKORDB_ARGS=--port $((6379 + i - 1)) --cluster-enabled yes --cluster-announce-ip node$i --cluster-announce-port $((6379 + i - 1))" \
falkordb/falkordb
done
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and persistence configuration.

The node startup script could be improved in several areas:

  1. Add validation for Docker network existence
  2. Include error handling for container creation
  3. Add volume mounts for data persistence

Consider updating the script:

 for i in {1..6}; do
+  # Validate network exists
+  if ! docker network inspect falkordb-cluster-network >/dev/null 2>&1; then
+    echo "Error: falkordb-cluster-network not found"
+    exit 1
+  fi
+
   docker run -d \
     --name node$i \
     --hostname node$i \
     --network falkordb-cluster-network \
     -p $((6379 + i - 1)):$((6379 + i - 1)) \
     -e BROWSER=0 \
+    -v falkordb-data-$i:/data \
     -e "FALKORDB_ARGS=--port $((6379 + i - 1)) --cluster-enabled yes --cluster-announce-ip node$i --cluster-announce-port $((6379 + i - 1))" \
-    falkordb/falkordb
+    falkordb/falkordb || {
+      echo "Error: Failed to start node$i"
+      exit 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
for i in {1..6}; do
docker run -d \
--name node$i \
--hostname node$i \
--network falkordb-cluster-network \
-p $((6379 + i - 1)):$((6379 + i - 1)) \
-e BROWSER=0 \
-e "FALKORDB_ARGS=--port $((6379 + i - 1)) --cluster-enabled yes --cluster-announce-ip node$i --cluster-announce-port $((6379 + i - 1))" \
falkordb/falkordb
done
for i in {1..6}; do
# Validate network exists
if ! docker network inspect falkordb-cluster-network >/dev/null 2>&1; then
echo "Error: falkordb-cluster-network not found"
exit 1
fi
docker run -d \
--name node$i \
--hostname node$i \
--network falkordb-cluster-network \
-p $((6379 + i - 1)):$((6379 + i - 1)) \
-e BROWSER=0 \
-v falkordb-data-$i:/data \
-e "FALKORDB_ARGS=--port $((6379 + i - 1)) --cluster-enabled yes --cluster-announce-ip node$i --cluster-announce-port $((6379 + i - 1))" \
falkordb/falkordb || {
echo "Error: Failed to start node$i"
exit 1
}
done
🧰 Tools
🪛 GitHub Actions: spellcheck

[warning] Spelling error found: 'hostnames'


[warning] Spelling error found: 'testGraph'

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
operations/cluster.md (2)

37-46: 🛠️ Refactor suggestion

Add error handling and data persistence.

The node startup script needs improvements:

  1. Add error handling for Docker operations
  2. Configure data persistence
  3. Document the purpose of BROWSER=0
  4. Add port range validation

Apply these improvements:

 for i in {1..6}; do
+  # Validate port range
+  if [ $((6379 + i - 1)) -gt 65535 ]; then
+    echo "Error: Port number exceeds maximum allowed (65535)"
+    exit 1
+  fi
+
   docker run -d \
     --name node$i \
     --hostname node$i \
     --network falkordb-cluster-network \
     -p $((6379 + i - 1)):$((6379 + i - 1)) \
     -e BROWSER=0 \
+    # Mount a volume for data persistence
+    -v falkordb-data-$i:/data \
     -e "FALKORDB_ARGS=--port $((6379 + i - 1)) --cluster-enabled yes --cluster-announce-ip node$i --cluster-announce-port $((6379 + i - 1))" \
-    falkordb/falkordb
+    falkordb/falkordb || {
+      echo "Error: Failed to start node$i"
+      exit 1
+    }
 done

Also, please add documentation explaining the purpose of the BROWSER=0 environment variable.

🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error found: 'hostnames' was flagged as misspelled


99-105: 🛠️ Refactor suggestion

Improve new node setup and add rebalancing instructions.

The new node setup needs improvements:

  1. Add error handling and persistence
  2. Use variables for better maintainability
  3. Add cluster rebalancing steps

Apply these improvements:

+# Calculate next available node number and port
+node_num=7
+port=$((6379 + node_num - 1))
+
 docker run -d \
-    --name node7 \
-    --hostname node7 \
+    --name "node${node_num}" \
+    --hostname "node${node_num}" \
     --network falkordb-cluster-network \
-    -p 6385:6385 \
+    -p ${port}:${port} \
     -e BROWSER=0 \
-    -e "FALKORDB_ARGS=--port 6385 --cluster-enabled yes --cluster-announce-ip node7 --cluster-announce-port 6385" \
+    -v "falkordb-data-${node_num}:/data" \
+    -e "FALKORDB_ARGS=--port ${port} --cluster-enabled yes --cluster-announce-ip node${node_num} --cluster-announce-port ${port}" \
     falkordb/falkordb

Add rebalancing instructions after node addition:

# Rebalance the cluster to distribute slots to the new node
docker exec -it node1 redis-cli --cluster rebalance node1:6379

# Monitor rebalancing progress
docker exec -it node1 redis-cli --cluster check node1:6379
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error found: 'hostnames' was flagged as misspelled

🧹 Nitpick comments (2)
operations/cluster.md (2)

69-69: Add node validation and explain cluster configuration.

The cluster creation command needs improvements:

  1. Add node availability check
  2. Explain replication configuration
  3. Provide failure handling guidance

Add these improvements before the cluster creation command:

# Validate that all nodes are available
for i in {1..6}; do
  if ! docker exec -it node$i redis-cli ping > /dev/null; then
    echo "Error: node$i is not responding"
    exit 1
  fi
done

# Create the cluster
# --cluster-replicas 1 configures one replica for each master node,
# resulting in 3 master-replica pairs for high availability
docker exec -it node1 redis-cli --cluster create \
  node1:6379 node2:6380 node3:6381 \
  node4:6382 node5:6383 node6:6384 \
  --cluster-replicas 1 --cluster-yes

# If cluster creation fails:
# 1. Check node logs: docker logs nodeN
# 2. Reset cluster state: docker exec -it nodeN redis-cli CLUSTER RESET
# 3. Retry cluster creation
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error found: 'hostnames' was flagged as misspelled


118-119: Consolidate hosts file update logic.

The hosts file update for the new node duplicates the earlier logic. Consider creating a reusable function.

Move this to a function at the beginning of the guide:

# Add this function after the prerequisites section
update_hosts() {
  local node_num=$1
  local backup_needed=${2:-false}
  
  # Backup hosts file if needed
  if [ "$backup_needed" = true ] && ! sudo cp /etc/hosts /etc/hosts.backup; then
    echo "Error: Failed to backup hosts file. Please check sudo permissions."
    return 1
  }
  
  # Add node to hosts file
  if ! echo "127.0.0.1 node${node_num}" | sudo tee -a /etc/hosts > /dev/null; then
    echo "Error: Failed to update /etc/hosts for node${node_num}"
    [ "$backup_needed" = true ] && sudo mv /etc/hosts.backup /etc/hosts
    return 1
  }
  return 0
}

# Usage examples:
# Initial nodes: for i in {1..6}; do update_hosts $i true; done
# New node: update_hosts 7
🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error found: 'hostnames' was flagged as misspelled

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7d47bd and 9ceaa80.

📒 Files selected for processing (1)
  • operations/cluster.md (2 hunks)
🧰 Additional context used
🪛 GitHub Actions: spellcheck
operations/cluster.md

[error] Spelling error found: 'hostnames' was flagged as misspelled

🔇 Additional comments (1)
operations/cluster.md (1)

54-56: 🛠️ Refactor suggestion

Improve hosts file modification safety and efficiency.

The current implementation has several issues:

  1. No error handling for sudo permissions
  2. Inefficient use of multiple sudo commands
  3. No backup of hosts file before modification

Apply these improvements:

+# Backup hosts file
+if ! sudo cp /etc/hosts /etc/hosts.backup; then
+  echo "Error: Failed to backup hosts file. Please check sudo permissions."
+  exit 1
+fi
+
+# Add all nodes in a single operation
+nodes_config=$(for i in {1..6}; do echo "127.0.0.1 node$i"; done)
+if ! echo "$nodes_config" | sudo tee -a /etc/hosts > /dev/null; then
+  echo "Error: Failed to update /etc/hosts. Restoring backup..."
+  sudo mv /etc/hosts.backup /etc/hosts
+  exit 1
+fi
-for i in {1..6};do
-  sudo echo "127.0.0.1 node$i" | sudo tee -a /etc/hosts
-done

Likely invalid or redundant comment.

🧰 Tools
🪛 GitHub Actions: spellcheck

[error] Spelling error found: 'hostnames' was flagged as misspelled

@swilly22 swilly22 merged commit e5a5392 into main Jan 5, 2025
2 checks passed
@swilly22 swilly22 deleted the 109-update-documentation-as-to-how-to-create-a-falkordb-cluster branch January 5, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update documentation as to how to create a FalkorDB cluster
2 participants