-
Notifications
You must be signed in to change notification settings - Fork 335
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
[ PR-1071-2] add sendRequest.done() to release resource together #1110
Conversation
57209ec
to
63fcefc
Compare
This PR looks good to me, and looks remove duplicate code from Please ask other committers to help review. |
@nodece If you apporve this patch to be merged, please give a formal review reaction :D |
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.
Let's go forward. Although, I don't like this spilt flavor that we add methods or fields that is unused in the patch - each patch should be self-contained and we add methods or fields when we do use it.
Given #1071 pending for a long time and I review the following PR that use these fields and methods, I can merge this one. But if it's possible please try to make patches self-contained instead of adding something unused first and letting the reviewers guess how it will be used.
@@ -1419,21 +1419,37 @@ func (p *partitionProducer) Close() { | |||
<-cp.doneCh | |||
} | |||
|
|||
//nolint:all |
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.
Ensure we remove this nolint
in a following patch.
@@ -1443,6 +1459,55 @@ func (sr *sendRequest) stopBlock() { | |||
}) | |||
} | |||
|
|||
//nolint:all |
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.
ditto
)" This needs a rework. This reverts commit f9969ca.
(If this PR fixes a github issue, please add
Fixes #<xyz>
.)Fixes #1071
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>
to link to the master issue.)Master Issue: #1071
Motivation
Spilt PR #1071 into multiple ones, this is the SECOND one.
#1071 is a BIG PR, it refactor the sending logic, here we just add fields to
sendRequest
addsendRequest.done()
to makesendRequest
can keep track of the resouces it applied and can release them together.Modifications
Describe the modifications you've done.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation