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

Support for Multiple Endpoints #1083

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

vazois
Copy link
Collaborator

@vazois vazois commented Mar 10, 2025

This PR adds support for multiple endpoints through --bind option.

@prvyk
Copy link
Contributor

prvyk commented Mar 10, 2025

You can use several different loopback addresses together for the test and skip dealing with Dns/network interfaces. IPv4 has the entire 127.x.x.x range for this.

@vazois vazois force-pushed the vazois/multi-endpoints branch from 208bb46 to a5f6025 Compare March 10, 2025 21:37
@vazois
Copy link
Collaborator Author

vazois commented Mar 10, 2025

You can use several different loopback addresses together for the test and skip dealing with Dns/network interfaces. IPv4 has the entire 127.x.x.x range for this.

Thanks for the suggestion. I want to test also the non-loopback addresses. So, added test for loopback ipv4 and ipv6

@vazois vazois force-pushed the vazois/multi-endpoints branch 2 times, most recently from 3713e9e to 8126dd1 Compare March 11, 2025 21:59
@vazois vazois marked this pull request as ready for review March 11, 2025 22:01
@Copilot Copilot bot review requested due to automatic review settings March 11, 2025 22:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for multiple endpoints through the --bind option by refactoring various modules to work with endpoint arrays instead of a single endpoint. Key changes include updating the configuration and validation functions (e.g. in Format.cs and Options.cs), adapting server initializations (e.g. in GarnetServer and StoreWrapper), and modifying test cases to use endpoint arrays.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/Garnet.test/GarnetServerConfigTests.cs Adds a new test to validate multi-endpoint support.
libs/common/Format.cs Refactors endpoint parsing functions to return arrays and updates error messaging.
libs/server/Metrics/Info/GarnetInfoMetrics.cs Adjusts metrics retrieval to support multiple endpoints.
libs/host/Configuration/OptionsValidators.cs, Options.cs Updates configuration options for multiple endpoints.
test/Garnet.test/TestUtils.cs Updates test utilities to work with endpoint arrays.
libs/server/Resp/PurgeBPCommand.cs, libs/server/Servers/ServerOptions.cs, libs/cluster/Server/... Various updates to replace single endpoint properties with endpoint arrays.
test/Garnet.test/UnixSocketTests.cs, GarnetClientTests.cs, benchmark/... Adjusts tests and benchmarks to align with multi-endpoint changes.

@prvyk
Copy link
Contributor

prvyk commented Mar 11, 2025

This all works, just a few notes (IMHO it would be reasonable to save them for a future PR):

  • Error message when failing to bind to an address is IMHO a bit worse now. Better to not print out the stacktrace by not passing the exception to LogError.
  • RedisConf import currently uses only first bind address and does not yet support multiple endpoints.
  • Redis allows creating a Unix socket concurrently. ('--bind 127.0.0.1 --unixsocket ./socket'. In Redis both are defined, here only the socket).

@vazois vazois force-pushed the vazois/multi-endpoints branch from 05ca313 to cef3bdc Compare March 12, 2025 01:50
@vazois
Copy link
Collaborator Author

vazois commented Mar 12, 2025

This all works, just a few notes (IMHO it would be reasonable to save them for a future PR):

  • Error message when failing to bind to an address is IMHO a bit worse now. Better to not print out the stacktrace by not passing the exception to LogError.
  • RedisConf import currently uses only first bind address and does not yet support multiple endpoints.
  • Redis allows creating a Unix socket concurrently. ('--bind 127.0.0.1 --unixsocket ./socket'. In Redis both are defined, here only the socket).

I made a change to log the message instead of the stack trace for the first one.
Not 100% positive on this but the other two suggestions seem to require a little bit more work that might be better done at another PR. Especially the last one. I will check for the second one.

@badrishc
Copy link
Collaborator

Unix socket and TCP socket at the same time was, I thought, the use case that first drove this feature. Would be useful to see what makes that harder than two TCP sockets.

@vazois
Copy link
Collaborator Author

vazois commented Mar 12, 2025

Unix socket and TCP socket at the same time was, I thought, the use case that first drove this feature. Would be useful to see what makes that harder than two TCP sockets.

The original purpose of this PR was to fully support hostnames that could map to multiple interfaces when provided through the bind option.
I am wondering if supporting unix sockets is as a simple as creating another instance of GarnetServerTcp.
If so, why we did not do this in the original PR? @PaulusParssinen

@badrishc
Copy link
Collaborator

I am wondering if supporting unix sockets is as a simple as creating another instance of GarnetServerTcp. If so, why we did not do this in the original PR? @PaulusParssinen

I think it should be as simple as creating another instance of GarnetServerTcp with that endpoint (@PaulusParssinen?) .. We did not support multiple GarnetServerTcp instances until this PR, so it was not done earlier, IIRC.

@vazois vazois force-pushed the vazois/multi-endpoints branch 2 times, most recently from 2d53a7d to 91c99bc Compare March 12, 2025 19:57
@PaulusParssinen
Copy link
Contributor

I am wondering if supporting unix sockets is as a simple as creating another instance of GarnetServerTcp. If so, why we did not do this in the original PR? @PaulusParssinen

I think it should be as simple as creating another instance of GarnetServerTcp with that endpoint (@PaulusParssinen?) .. We did not support multiple GarnetServerTcp instances until this PR, so it was not done earlier, IIRC.

The reason I didn't try to do this all in one PR (the one that added support for unix socket) is that I don't have confident mental model for what sort of changes that could possibly entail and hoped that just like this PR does, someone who has much clearer picture of the cluster mode (and the current networking stack) would have stab at it instead (Thanks!).

Glad to see it was as simple as making many GarnetServerTcp instances 🎉

@TalZaccai TalZaccai requested review from badrishc and TalZaccai March 13, 2025 18:32
@vazois vazois force-pushed the vazois/multi-endpoints branch 3 times, most recently from e942f80 to d208c8a Compare March 20, 2025 20:56
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Added some config-related comments...

@vazois vazois force-pushed the vazois/multi-endpoints branch from 7314751 to 7dd95ed Compare March 20, 2025 21:54
@vazois vazois force-pushed the vazois/multi-endpoints branch from 6d58f56 to e08ebe6 Compare March 21, 2025 16:19
@vazois vazois requested a review from TalZaccai March 21, 2025 17:03
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.

5 participants