-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Agent connection pool cleanup #7966
Conversation
f2b8739
to
616819b
Compare
The version field has been used to decide which multiplexing to use. It was introduced in 2457293. But this is 6y ago and there is no need for this differentiation anymore.
616819b
to
a1e835a
Compare
In the past TLS usage was enforced with these variables, but these days this decision is made by TLSConfigurator and there is no reason to keep using the variables.
Timeout was never used in a meaningful way by callers, which is why it is now entirely internal to the pool.
a1e835a
to
1fbc1d4
Compare
agent/pool/pool.go
Outdated
var codec rpc.ClientCodec | ||
conn, _, err := p.DialTimeoutInsecure(dc, nodeName, addr, 1*time.Second, p.TLSConfigurator.OutgoingRPCWrapper()) | ||
conn, _, err := p.dial(dc, nodeName, addr, 1*time.Second, 0, RPCTLSInsecure) |
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.
The second to last arg is zero because it doesn't matter what the value is, right? In the prior invocation RPCTLSInsecure was passed for both values (and the second-to-last value was still similarly ignored).
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.
Yes! dial
ignores the second to last value if it is in RPCTLSInsecure
mode:
Lines 371 to 376 in 1fbc1d4
if tlsRPCType != RPCTLSInsecure { | |
if _, err := conn.Write([]byte{byte(actualRPCType)}); err != nil { | |
conn.Close() | |
return nil, nil, err | |
} | |
} |
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.
LGTM
Agent connection pool cleanup
Removes some of the cruft from agent connection pool. Best viewed by commit.