-
Notifications
You must be signed in to change notification settings - Fork 468
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
Return the behaviour of a new default GRPC client instantiation for every request #3130
Return the behaviour of a new default GRPC client instantiation for every request #3130
Conversation
|
Welcome @ciarams87! |
Hi @ciarams87. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
thanks for the fix @ciarams87, the changes lgtm! /ok-to-test |
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 for catching this, @ciarams87!
I have shared some thoughts, though perhaps we may want to address those independently of this PR (and not necessarily by you)
/ok-to-test
@@ -270,6 +270,9 @@ func validateExpectedResponse(t *testing.T, expected ExpectedResponse) { | |||
func MakeRequestAndExpectEventuallyConsistentResponse(t *testing.T, c Client, timeoutConfig config.TimeoutConfig, gwAddr string, expected ExpectedResponse) { | |||
t.Helper() | |||
validateExpectedResponse(t, expected) | |||
if c == nil { | |||
c = &DefaultClient{Conn: nil} | |||
} | |||
defer c.Close() |
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.
While the change does help address the immediate concern, it does seem to reveal a flaw here -- I would expect the client to only be closed by MakeRequestAndExpectEventuallyConsistentResponse()
when it was instantiated within MakeRequestAndExpectEventuallyConsistentResponse()
itself.
When a non-nil client is passed to the MakeRequestAndExpectEventuallyConsistentResponse()
, this function should not own the lifecycle the passed-clients lifecycle.
An implication of this would be that the Client interface should not have the Close
function.
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 for the review @gauravkghildiyal!
When a non-nil client is passed to the MakeRequestAndExpectEventuallyConsistentResponse(), this function should not own the lifecycle the passed-clients lifecycle.
An implication of this would be that the Client interface should not have the Close function.
That makes a lot of sense to me, but I'm not entirely sure that should be addressed in this PR - the scope here is only to return the DefaultClient behaviour to how it was before. I'd like to defer to @snehachhabria as the original author of #3095 as to the decisions made in this respect.
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.
@gauravkghildiyal what you suggested should be done and can be handled in a separate PR. Once this PR is merged, I can handle the suggested changes.
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 @ciarams87 and @snehachhabria!
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.
This LGTM (has follow-up items which will be tackled separately).
I see @arkodg on the reviewers list who can help with conformance approval.
/lgtm
@@ -270,6 +270,9 @@ func validateExpectedResponse(t *testing.T, expected ExpectedResponse) { | |||
func MakeRequestAndExpectEventuallyConsistentResponse(t *testing.T, c Client, timeoutConfig config.TimeoutConfig, gwAddr string, expected ExpectedResponse) { | |||
t.Helper() | |||
validateExpectedResponse(t, expected) | |||
if c == nil { | |||
c = &DefaultClient{Conn: nil} | |||
} | |||
defer c.Close() |
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 @ciarams87 and @snehachhabria!
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.
/approve
/lgtm
thanks !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkodg, ciarams87, gnossen, snehachhabria The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/area conformance
What this PR does / why we need it:
Returns the GRPCRoute conformance tests default client to instantiating a new client for every request, instead of creating one shared client for all requests, as this is leading to conflicts and closed client connections. More details in the issue: #3122
Which issue(s) this PR fixes:
Fixes #3122
Does this PR introduce a user-facing change?: