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

grpc: Add join Dial Option #5861

Merged
merged 2 commits into from
Dec 17, 2022
Merged

grpc: Add join Dial Option #5861

merged 2 commits into from
Dec 17, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Dec 13, 2022

This PR adds a join Dial Option. It also contains tests for this join Dial Option and the already present join Server Option.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley December 13, 2022 02:37
@zasweq zasweq added the Type: Feature New features or improvements in behavior label Dec 13, 2022
@zasweq zasweq added this to the 1.52 Release milestone Dec 13, 2022
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM except the two nits.

Comment on lines 29 to 30
const maxRecvSize = 998765
const initialWindowSize = 100
Copy link
Member

Choose a reason for hiding this comment

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

Please make these local inside their tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used 4/2 times, respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is in grpc I see I'll move to tests.

@@ -77,6 +77,9 @@ var (
// ClearGlobalDialOptions clears the array of extra DialOption. This
// method is useful in testing and benchmarking.
ClearGlobalDialOptions func()
// JoinDialOptions combines the dial options passed as arguments into a
// single dial option.
JoinDialOptions interface{}
Copy link
Member

Choose a reason for hiding this comment

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

Please comment: // func(...grpc.DialOption) grpc.DialOption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley Dec 14, 2022
@dfawley dfawley modified the milestones: 1.52 Release, 1.53 Release Dec 16, 2022
@dfawley dfawley closed this Dec 17, 2022
@dfawley dfawley reopened this Dec 17, 2022
@zasweq zasweq merged commit 54b7d03 into grpc:master Dec 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants