-
Notifications
You must be signed in to change notification settings - Fork 550
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
Add Unix socket support #944
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Going through it in iterations because it changes a lot of files.
Are we adding any explicit tests for unix socket?
I added single, a very primitive, test to check that the networking stacks of server and If you have any ideas for further tests, I'm happy to add them. |
Perfect, this is exactly what I was looking for. I am wondering if having also the same but with TLS on would make sense or if it is even possible. If you can add something like that, it will be great. |
this would be nice to have as part of unix socket support. separate PR is fine if it is a non-trivial addition. |
Hi @PaulusParssinen, when the unix socket is enabled, does that need to preclude normal TCP listeners and connections to the server? It seems we ought to be able to accept both (or just one if so desired) at the same time, i.e. enabling unix sockets should not preclude TCP connections. That way cluster mode would continue to work (using TCP, because cluster is usually remote servers) when unix sockets are enabled. E.g., in other systems, you would enable both endpoints as follows:
|
You're right, I think TLS path needs to be tested (or atleast how it fails and handle that if it is trivial to support ). I'll investigate 👍
Should be pretty trivial in hindsight, if we presumably gate this permission option for non-windows platforms 👍 (unless someone knows how to map unix permission octal to windows file permission!)
Originally I wrote in the PR description, to currently to keep the PR "simple", to make it temporarily either or decision => inherently makes cluster-mode not supported. This made the PR mostly mechanical This of course does not need to be case, but having two listener endpoints active (it'd be probably implemented to support any number of endpoints) from my viewpoint would singificantly raise the complexity of this PR as I think I'd have to spend quite bit more time to be confident to start refactoring the networking stack to accommodate this (potential races etc.). That's my rational for why I'd rather see this work be done as a follow up PR. This PR lays foundation for this follow-up refactor imo. |
This makes sense. Now that we have the clean abstraction of endpoints, the introduction of multiple endpoints should be doable as a separate work item, as that is orthogonal to this PR. |
This PR adds initial unix socket support to Garnet. The unix domain socket path can be specified with
--unixsocket
switch. Becausestring address, int port
pair was used everywhere, this change required a lot of tiny changes in testing and networking logic. There is more clean-up work left in some parts of the code, e.g. usingIPEndPoint
to represent cluster nodes instead of the(string address, int port)
tuple.I'm leaving implementation of--unixsocketperm
switch to be done as a follow-up.--unixsocketperm
is now implemented for applicable platforms.I'm also for now making the server initialization throw
NotImplementedException
if user attempts to specify unix socket listener in cluster mode. Supporting cluster mode would need bit more work to have two different types of listener endpoints active.Contributes to #682