-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat: update tcp client connections #1429
Conversation
Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
func BuildConnAddrs(numCpu int) []string { | ||
// buildConnAddrs Populate the connection list for the clients | ||
// Format (serverAddr, serverIdx) : (0.0.0.0:5551, 1), (0.0.0.0:5552, 2) | ||
func buildConnAddrs(numCpu int, servPorts []string) []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.
numCpu
could be potentially not equal to len(servPorts)
, so the logic below is risky.
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.
From the SDK side they will both be the same value as numCpu is written to the server info as the number of server instances required in multiproc.
But I can actually do away with the variable itself and just use servPorts and len(servPorts), I guess would be easier for understanding
Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
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.
We could have used UDS (multiple ones) to achieve the same? We used TCP because of lack of support for multiplexing in UDS.
Updates the grpc client to use multiple sockets for TCP connections instead of a single port with reuse