marceloboeira
feature: Add UNIX socket support
I need some help here:
- I could not figure out how to test the
socketabstraction.spawnin thespec_helpersuses a real address. I don't know if I could just change it to another and let it go. - For some reason the test for UNIXServer is failing (but that I can figure out myself)
- How would the
cliwork if everything needs a default value? Should not we handle hostname, port, unix file as justaddress?
Thank you!
@mauricioabreu the code is quite nice so far, I will take a proper look tonight and try to answer your questions!
No, thank you for all the support.
:)
On Fri, Sep 2, 2016 at 10:13 AM Mauricio de Abreu Antunes < [email protected]> wrote:
@marceloboeira https://github.com/marceloboeira Thank you!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/marceloboeira/bojack/pull/49#issuecomment-244311795, or mute the thread https://github.com/notifications/unsubscribe-auth/ABz28V_tPiJ6TvuGD1l-5FeRhMNpvRSRks5ql9q0gaJpZM4JzL-U .
Marcelo Boeira
I was reading a book these days, "Practical Object-Oriented Design in Ruby". In the beginning of this book the author talks about the case where we create functions that need hostname and port and she asks why we don't call it just address.
@mauricioabreu the thing is, you can have the hostname and port together, for sure. BUT you have to consider you are adding a responsibility to the class that receives the address, to split it. Since we don't use as an address, now this responsibility is credited to the "user".
If you add this logic and continue to do this for the dependencies, you make the server class handle a lot of functions, which is not what is supposed to do.
The best way to do it, in my opinion, would be to have a BoJack::Address class, that receives the input and return hostname and port. The server receives the address object instead of 2 strings.
@marceloboeira I was not thinking in adding this code to the server. Just like you said an Address object could handle it. We just use the address to create the socket and logging. Not sure if we actually use the hostname and port for specific cases.
Anyways, I am kinda lost with this PR. Commander needs a default value for every argument. This way I can not check if the user is trying to connect via TCP/IP or UnixSocket.
I am trying to solve this issue without this refactor (I am not sure if we need to have this refactor).
Reading the redis CLI it uses hostname and port (-h and -p)
Well, one idea is to have a default "empty" for the Unix socket since it is not the default way to run BoJack.
Basically, what we need is to decide the default interface: Unix/TCP.
My opinion, TCP should be the default, then we could have a flag: "--unix", "--use-unix-socket", if the flag is true then hostname as port are ignored.
What do you think? less effort and it is backwards compatible.
@marceloboeira I agree that TCP should be default.
The problem here is that other parts of the codebase rely on "hostname/port" like BoJack::Logger.build
@mauricioabreu hmm, maybe it is not relevant at all to have the hostname and port in the log after all.
@marceloboeira For developers/teams who watch logs and take actions on behalf of them it is useful. Does it make sense?
@mauricioabreu in order to make it simple, create a logger initializer that initializes with the socket path, for another formatter, so a formatter for TCP and another for UnixSocket.
Thanks for sharing @marceloboeira
redis-cli does it by having a unix-socket-file argument.
I am still trying to solve the code which passes the right socket ahead (UNIXServer or TCPServer). After solving this problem I will get back to the CLI.
@mauricioabreu yes, I think the unix-socket-file it is better. I just found interesting the way PGSQL does and wanted to document it here.
@marceloboeira thank you! It is good too see how others are handling this problem.
@mauricioabreu no worries, I am actually testing something with the webhooks, that's why I closed and reopened ahha
@marceloboeira haha :P I am studying crystal again. Maybe it is time to get back to this pull request