feat: add UDP support #20
Conversation
|
How it looks like locally and with adjusted log level for the server: Some notes:
We were not consistent in how we call our messages: Sometimes we refer to message, probe or data. Another PR could make it more consistent, but out of scope for now. WDYT? |
IMO those three are different things, the message is the body of the probe, the probe is the packet sent over the network, and data is a generic word that could be used to talk about both of those things. We probably do need a quick round of renamings for consistency if those are scrambled currently, in another PR then yes :) |
cmd/connqc/client.go
Outdated
| var protocol string | ||
| switch { | ||
| case c.Bool(flagProtocolTCP) && !c.Bool(flagProtocolUDP): | ||
| protocol = "tcp" | ||
| case c.Bool(flagProtocolUDP) && !c.Bool(flagProtocolTCP): | ||
| protocol = "udp" | ||
| default: | ||
| return fmt.Errorf("either --%s or --%s must be set", flagProtocolTCP, flagProtocolUDP) | ||
| } | ||
| log = log.With(lctx.Str("protocol", protocol)) |
There was a problem hiding this comment.
Wouldn't it be simpler to have one --protocol flag that would be required and allow us to choose between the two protocols? Do we ever want to start both at the same time? If we do, it would be clearer to do it using two connqc processes, WDYT?
Either that or make the protocol a subcommand instead of a flag, i.e. ./server udp --addr=<xxx> / ./server tcp --addr=<xxx>.
There was a problem hiding this comment.
There was a discussion around this in Slack (without your input, so you might have missed that?), where we agreed on this approach, at least from my understanding.
Now that I think about it, I mainly voted for --udp & --tcp to allow both connections at once, which is not supported in this implementation haha. As @DrPsychick also insisted that --tcp & --udp "makes it more complicated", I will update this to --protocol with a default to tcp.
There was a problem hiding this comment.
I think we concluded that there are use cases where one would want to send/receive UDP and TCP on the same port, but I would do this in a separate PR now. If one needs to test both, one can run separate instances on 2 different ports.
There was a problem hiding this comment.
The server accepts TCP and UDP on the same port, these params were just for the client.
There was a problem hiding this comment.
As you prefer, I just thought this could simplify things but if there is a justification for it I am fine with it 👍
In the future make sure to talk about those things on the issue rather than on Slack though, to make the repository more transparent/open 😁
There was a problem hiding this comment.
Just for clarification: It is now updated to use --protocol=tcp / --protocol=udp on the client side. The server already worked for both. See bc63aed.
…l/buffr.BufferedReader
Goal of this PR
Resolves #10
Commit-based review recommended.
Checklist
Does the PR require tests to be added or updated?
Yes
Misc
main)