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.

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

enhance: Decouple shard client manager from shard cache (#37371) #37753

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

weiliu1031
Copy link
Contributor

issue: #37115
pr: #37371 #37646 #37729
the old implementation update shard cache and shard client manager at same time, which causes lots of conor case due to concurrent issue without lock.

This PR decouple shard client manager from shard cache, so only shard cache will be updated if delegator changes. and make sure shard client manager will always return the right client, and create a new client if not exist. in case of client leak, shard client manager will purge client in async for every 10 minutes.


)

issue: milvus-io#37115
the old implementation update shard cache and shard client manager at
same time, which causes lots of conor case due to concurrent issue
without lock.

This PR decouple shard client manager from shard cache, so only shard
cache will be updated if delegator changes. and make sure shard client
manager will always return the right client, and create a new client if
not exist. in case of client leak, shard client manager will purge
client in async for every 10 minutes.

---------

Signed-off-by: Wei Liu <[email protected]>
@sre-ci-robot sre-ci-robot added the size/XL Denotes a PR that changes 500-999 lines. label Nov 17, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Nov 17, 2024
congqixia and others added 4 commits November 17, 2024 23:43
This PR refine the shard client ref counter, dec ref counter won't
release client anymore, and only permit shard client manager to remove client.

Signed-off-by: Wei Liu <[email protected]>
Signed-off-by: Wei Liu <[email protected]>
Signed-off-by: Wei Liu <[email protected]>
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 80.81395% with 33 lines in your changes missing coverage. Please review.

Project coverage is 79.65%. Comparing base (13f83df) to head (919654a).
Report is 25 commits behind head on 2.4.

Files with missing lines Patch % Lines
internal/proxy/shard_client.go 78.94% 14 Missing and 6 partials ⚠️
internal/proxy/meta_cache.go 62.50% 12 Missing ⚠️
internal/proxy/roundrobin_balancer.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              2.4   #37753      +/-   ##
==========================================
+ Coverage   72.28%   79.65%   +7.36%     
==========================================
  Files        1069     1069              
  Lines      166874   166869       -5     
==========================================
+ Hits       120619   132912   +12293     
+ Misses      41853    29544   -12309     
- Partials     4402     4413      +11     
Files with missing lines Coverage Δ
internal/proxy/lb_policy.go 98.03% <100.00%> (+0.13%) ⬆️
internal/proxy/look_aside_balancer.go 96.60% <100.00%> (+0.11%) ⬆️
internal/proxy/task_policies.go 86.20% <100.00%> (ø)
internal/proxy/roundrobin_balancer.go 76.92% <0.00%> (-6.42%) ⬇️
internal/proxy/meta_cache.go 90.15% <62.50%> (-1.29%) ⬇️
internal/proxy/shard_client.go 73.80% <78.94%> (-8.80%) ⬇️

... and 251 files with indirect coverage changes

---- 🚨 Try these New Features:

@mergify mergify bot added the ci-passed label Nov 17, 2024
@mergify mergify bot removed the ci-passed label Nov 18, 2024
@weiliu1031
Copy link
Contributor Author

rerun ut

@mergify mergify bot added the ci-passed label Nov 18, 2024
@czs007
Copy link
Collaborator

czs007 commented Nov 25, 2024

/approve
/lgtm

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czs007, weiliu1031

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot removed the ci-passed label Nov 25, 2024
@yanliang567 yanliang567 added ci-passed manual-pass manually set pass before ci-passed labeled labels Nov 25, 2024
@sre-ci-robot sre-ci-robot merged commit b245101 into milvus-io:2.4 Nov 25, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm manual-pass manually set pass before ci-passed labeled size/XL Denotes a PR that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants