Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for Unix sockets #86

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Add support for Unix sockets #86

merged 1 commit into from
Sep 24, 2019

Conversation

Daniel-B-Smith
Copy link
Contributor

We report a lot of our stats over Unix sockets, so I'm adding a new sink that will accept UnixStreams instead of UdpSockets. Arguably, it should just accept anything that implements the Write trait, which I'm happy to do as that is a pretty small refactoring.

Creating the draft PR early mostly for my own edification.

@56quarters
Copy link
Owner

Thanks for the PR! I think it's fine to create a sink solely for Unix sockets.

@Daniel-B-Smith
Copy link
Contributor Author

Thanks for the quick response! This should be ready to review now.

@Daniel-B-Smith Daniel-B-Smith marked this pull request as ready for review September 23, 2019 19:28
@56quarters 56quarters self-assigned this Sep 23, 2019
56quarters
56quarters requested changes Sep 23, 2019
View reviewed changes
Copy link
Owner

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Few minor changes noted. After addressing them, would you mind rebasing to a single commit before merge? Thanks!

src/client.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/sinks/uds.rs Show resolved Hide resolved
src/sinks/uds.rs Show resolved Hide resolved
Daniel-B-Smith
Daniel-B-Smith commented Sep 24, 2019
View reviewed changes
Copy link
Contributor Author

@Daniel-B-Smith Daniel-B-Smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the updates and squashed the commits.

src/client.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/sinks/uds.rs Show resolved Hide resolved
@56quarters 56quarters merged commit 42eacc0 into 56quarters:master Sep 24, 2019
@56quarters
Copy link
Owner

Awesome, merged!

@56quarters
Copy link
Owner

56quarters commented Sep 25, 2019 •
edited
Loading

Hmm, it seems like this might be the wrong UDS primitive (stream vs datagram).

According to these:

...the datagram protocol is used, not the stream protocol. This would mean using UnixDatagram in Rust instead of UnixStream.

Edit: opened #88

@56quarters 56quarters mentioned this pull request Sep 25, 2019
Closed
@Daniel-B-Smith
Copy link
Contributor Author

Yeah, you're correct. I just got ahead of myself. I'll send you the PR tomorrow.

@Daniel-B-Smith Daniel-B-Smith mentioned this pull request Sep 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@56quarters 56quarters 56quarters requested changes

Assignees

@56quarters 56quarters

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Daniel-B-Smith @56quarters