Skip to content

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jul 3, 2019

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.

@jpeach jpeach force-pushed the socket-option-names branch 3 times, most recently from 07155b1 to 5d8cb80 Compare July 3, 2019 11:57
Copy link
Member

@mattklein123 mattklein123 left a 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>>;
Copy link
Member

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.

Copy link
Contributor Author

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.

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>
@jpeach jpeach force-pushed the socket-option-names branch from b906007 to 43a6ef6 Compare July 9, 2019 08:48
@jpeach
Copy link
Contributor Author

jpeach commented Jul 9, 2019

@mattklein123 I switched from a tuple to a struct, and added an extra test for the option name. PTAL.

Copy link
Member

@mattklein123 mattklein123 left a 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>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants