-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(new source): Initial udp
source implementation
#738
Conversation
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
udp
source implementation
Test "topology::tests::topology_replace_source_transform_and_sink" is failing for some reason. |
@ktff that's a known intermittent failure that we're working to address (#694). If you re-run the tests it will most likely pass. cc/ @LucioFranco |
I've opened #740 in hopes that this will be fixed with our topology test updates. We also want to probably start limitnig more tests to use our |
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.
Looking pretty good! Just a few comments in line, let me know if you have any questions 😄
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! I agree with @LucioFranco's cleanups and that we can use the decoder and UdpFramed
better. Then this should be good to go 😄
src/sources/udp.rs
Outdated
.flatten() | ||
.map_err(|e: io::Error| error!("error reading datagram: {:?}", e)) | ||
.forward(out) | ||
.map(|_| info!("finished sending")) |
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.
Will this be triggered when we shut down the listening socket? I know we have something similar that gets hit when incoming tcp connections close. Not a big deal, but might be nice to be a little more specific with the message or just omit it if it's not really telling us something valuable (e.g. only happening on shutdown, which other log lines will be telling us about already).
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.
Those are errors from decoder. UdpFrame
it self throws no errors. So actually yes, I could be more specific about it.
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.
The finished sending
line? It looks like it will get triggered upon successful completion of the forward
future, which I think only happens once we stop listening on the socket.
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.
You were right in both occasions.
The first errors come from mio::UdpSocket
. They come from different methods, maybe even from OS, so message "error reading datagram: {:?}"
seems sensible to me.
The finished sending is indeed called only once, if even that since I am not sure that there is any other way of stop listening on udp socket besides droping the future.
Following commit will address this issues.
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
I don't have write permission to rerun testing. |
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.
I just reran the tests, but yes it'd be nice to figure out how to grant contributors access to that.
Unless the tests fail for some other reason or @LucioFranco has anything else, this looks good to me!
I took a look at UdpFramed and I see the issue...a bit annoying. I say we can merge this just fine and then introduce a fix to udpframed in 0.1 and get that published. Once that is done we can simplify this sink. |
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@lukesteensen do check latest commit that fixes the logging issue, after that I'll merge. |
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.
Looks perfect, thanks!
Nice work @ktff, looks great. |
I've opened an issue around UdpFramed that we should follow up with once it is done. ref tokio-rs/tokio#1443 |
Initial implementation of #338 plus tests.