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.

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

thriftbp: Support unix domain socket on thrift clients #649

Merged
merged 1 commit into from
May 7, 2024

Conversation

fishy
Copy link
Member

@fishy fishy commented May 3, 2024

Also update thrifttest to use UDS when possible.

@fishy fishy requested a review from a team as a code owner May 3, 2024 19:32
@fishy fishy requested review from kylelemons, konradreiche and pacejackson and removed request for a team May 3, 2024 19:32
Comment on lines 147 to 153
dir := cfg.TB.TempDir()
path := filepath.Join(dir, "socket")
socket = thrift.NewTServerSocketFromAddrTimeout(&net.UnixAddr{
Net: "unix",
Name: path,
}, 0)
addr = "uds://" + path
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this code will likely fail if ran on windows, but I don't think we care about windows users 🤷

if we do care about them, then we can extract this into 2 files with build conditions.

@fishy fishy force-pushed the thriftbp-client-uds branch from 3eba3e9 to f41c106 Compare May 3, 2024 22:21
@fishy fishy changed the title thriftbp: Support UDS on thrift clients thriftbp: Support unix domain socket on thrift clients May 3, 2024
Copy link
Contributor

@kylelemons kylelemons left a comment

Choose a reason for hiding this comment

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

Out of curiosity, what is the reason to prefer unix domain sockets over tcp for testing? I've always preferred TCP when that's what will be used in real code; the two are very similar but do not behave identically and the underlying libraries may even have different explicit behaviors.

I don't have a strong opinion here of course.

@fishy fishy force-pushed the thriftbp-client-uds branch from f41c106 to 059c28e Compare May 7, 2024 18:40
@fishy
Copy link
Member Author

fishy commented May 7, 2024

Out of curiosity, what is the reason to prefer unix domain sockets over tcp for testing? I've always preferred TCP when that's what will be used in real code; the two are very similar but do not behave identically and the underlying libraries may even have different explicit behaviors.

I don't have a strong opinion here of course.

I think make the test as close to real production as possible makes sense. Reverted that part of the changes.

@fishy fishy merged commit 4ae1e53 into reddit:master May 7, 2024
2 checks passed
@fishy fishy deleted the thriftbp-client-uds branch May 7, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants