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.

Sign up for GitHub

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

Jump to bottom

fix: Fix collection leak in querynode #36927

Closed
wants to merge 9 commits into from
Closed

fix: Fix collection leak in querynode #36927

wants to merge 9 commits into from

Conversation

bigsheeper
Copy link
Contributor

Remove the reference counting and replace it with the ReleaseCollection RPC.

issue: #36918

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Oct 16, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Oct 16, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 16, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@codecov Codecov
Copy link

codecov bot commented Oct 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.

Project coverage is 80.65%. Comparing base (8669153) to head (1bc342a).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
internal/querycoordv2/job/utils.go 81.25% 2 Missing and 1 partial ⚠️
internal/querycoordv2/session/cluster.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36927   +/-   ##
=======================================
  Coverage   80.64%   80.65%           
=======================================
  Files        1310     1310           
  Lines      183578   183578           
=======================================
+ Hits       148054   148056    +2     
+ Misses      30393    30390    -3     
- Partials     5131     5132    +1
Files with missing lines Coverage Δ
internal/querycoordv2/job/job_release.go 79.04% <100.00%> (+0.20%) ⬆️
internal/querycoordv2/services.go 85.26% <ø> (ø)
internal/querynodev2/segments/collection.go 93.00% <100.00%> (+0.39%) ⬆️
internal/querynodev2/services.go 91.39% <100.00%> (+0.45%) ⬆️
internal/querycoordv2/job/utils.go 81.63% <81.25%> (-0.08%) ⬇️
internal/querycoordv2/session/cluster.go 79.83% <75.00%> (-0.26%) ⬇️

... and 18 files with indirect coverage changes

@bigsheeper
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 16, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bigsheeper
To complete the pull request process, please assign yanliang567 after the PR has been reviewed.
You can assign the PR to them by writing /assign @yanliang567 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot added the ci-passed label Oct 17, 2024
@mergify mergify bot removed the ci-passed label Oct 18, 2024
weiliu1031
weiliu1031 reviewed Oct 18, 2024
View reviewed changes
internal/querynodev2/services.go Show resolved Hide resolved
internal/querycoordv2/job/job_release.go Outdated Show resolved Hide resolved
internal/querynodev2/services.go Show resolved Hide resolved
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 18, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@bigsheeper
Copy link
Contributor Author

/run-cpu-e2e

@bigsheeper
Copy link
Contributor Author

rerun ut

@mergify mergify bot added the ci-passed label Oct 21, 2024
@weiliu1031
Copy link
Contributor

/lgtm

czs007
czs007 reviewed Oct 22, 2024
View reviewed changes
internal/querynodev2/services.go
defer node.manager.Collection.Unref(req.GetCollectionID(), 1)
defer func() {
if !merr.Ok(status) {
node.manager.Collection.Release(req.GetCollectionID())
Copy link
Collaborator

Choose a reason for hiding this comment

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

should not Release, may release collection created by other segment.

@bigsheeper
Copy link
Contributor Author

If ReleaseCollection fails, it only logs a warning, which can still lead to a collection leak in the query node.

@bigsheeper bigsheeper closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiliu1031 weiliu1031 weiliu1031 left review comments

@czs007 czs007 czs007 left review comments

@aoiasd aoiasd Awaiting requested review from aoiasd

@congqixia congqixia Awaiting requested review from congqixia

Assignees

@weiliu1031 weiliu1031

Labels
ci-passed dco-passed DCO check passed. kind/bug Issues or changes related a bug lgtm size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bigsheeper @sre-ci-robot @weiliu1031 @czs007