feat(new source): Initial udp source implementation#738
feat(new source): Initial udp source implementation#738ktff merged 4 commits intovectordotdev:masterfrom ktff:udp-source
udp source implementation#738Conversation
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
binarylogic
changed the title
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 |
LucioFranco
left a comment
LucioFranco
left a comment
There was a problem hiding this comment.
Looking pretty good! Just a few comments in line, let me know if you have any questions 😄
binarylogic
requested review from
LucioFranco
and removed request for
lukesteensen
lukesteensen
left a comment
lukesteensen
left a comment
There was a problem hiding this comment.
Nice! I agree with @LucioFranco's cleanups and that we can use the decoder and UdpFramed better. Then this should be good to go 😄
| .flatten() | ||
| .map_err(|e: io::Error| error!("error reading datagram: {:?}", e)) | ||
| .forward(out) | ||
| .map(|_| info!("finished sending")) |
There was a problem hiding this comment.
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.
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.
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.
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. |
lukesteensen
left a comment
lukesteensen
left a comment
There was a problem hiding this comment.
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. |
|
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 |
4 participants
Initial implementation of #338 plus tests.