-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add socket option names for debugging. #7457
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
Conversation
07155b1
to
5d8cb80
Compare
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.
Thanks, in general this looks like a nice improvement. Can you check CI and also add some tests? Thank you!
/wait
// avoid proliferation of #ifdef. The first element is the sockopt | ||
// level, the second is the optname and the last is the human-readable | ||
// name for error logs. | ||
using SocketOptionName = absl::optional<std::tuple<int, int, std::string>>; |
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.
Mild preference for using a struct here vs. a tuple since IMO it's easier to read.
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.
Yeh I went tuple because it began with pair, but I agree a struct is better.
5d8cb80
to
407a3d4
Compare
407a3d4
to
b906007
Compare
Capture a human-readable name for a socket option so that if setting it fails, the log message can indicate which option was problematic. Signed-off-by: James Peach <jpeach@apache.org>
b906007
to
43a6ef6
Compare
@mattklein123 I switched from a tuple to a struct, and added an extra test for the option name. PTAL. |
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.
Thanks this is a nice improvement. 2 small comments. Also, friendly request to please not force push, it makes reviews much harder. Just add commits and we will squash when we merge. Thank you!
/wait
Signed-off-by: James Peach <jpeach@apache.org>
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.
Thank you!
Description:
Capture a human-readable name for a socket option so that if setting
it fails, the log message can indicate which option was problematic.
Risk Level:
Low
Testing:
Verified that trying to set the TCP fast open socket option to 15 on macOS logs which socket option failed.
Docs Changes:
None.
Release Notes:
None.