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

Placement service lock optimisations and race condition fix #7428

Merged
merged 14 commits into from Jan 26, 2024

Conversation

elena-kolevska
Copy link
Contributor

@elena-kolevska elena-kolevska commented Jan 22, 2024

Description

This PR introduces a few placement service optimisations:

  • Calculates the vnodes for all hosts outside of the placementTable lock
  • Calculates the vnodes for a single host outside of lock when adding a host to the placement table (which is still done for backwards compatibility)

A race condition fix:

  • The resetPlacementTables method was called without a lock from two places - on actor placement start and on reconnection to the placement service.

This PR It also clean ups the apiLevel parameter from the connectToServer method, because it's not used anymore.

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Unit tests passing
  • End-to-end tests passing

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (007ccf5) 62.31% compared to head (3eab372) 62.32%.

Files Patch % Lines
pkg/actors/placement/placement.go 83.33% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7428      +/-   ##
==========================================
+ Coverage   62.31%   62.32%   +0.01%     
==========================================
  Files         240      240              
  Lines       22070    22077       +7     
==========================================
+ Hits        13752    13760       +8     
+ Misses       7178     7173       -5     
- Partials     1140     1144       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska elena-kolevska marked this pull request as ready for review January 23, 2024 00:36
@elena-kolevska elena-kolevska requested review from a team as code owners January 23, 2024 00:36
@elena-kolevska elena-kolevska marked this pull request as draft January 23, 2024 00:54
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@@ -48,7 +48,7 @@ type placementClient struct {

// connectToServer initializes a new connection to the target server and if it succeeds replace the current
// stream with the connected stream.
func (c *placementClient) connectToServer(ctx context.Context, serverAddr string, apiLevel uint32) error {
func (c *placementClient) connectToServer(ctx context.Context, serverAddr string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a leftover cleanup from #7415

@elena-kolevska elena-kolevska changed the title Calculates vnode hashes outside of lock Placement service lock optimisations and race condition fix Jan 26, 2024
elena-kolevska and others added 2 commits January 26, 2024 12:30
…k inside of the method itself

Signed-off-by: Elena Kolevska <elena@kolevska.com>
@elena-kolevska elena-kolevska marked this pull request as ready for review January 26, 2024 12:43
Signed-off-by: Elena Kolevska <elena@kolevska.com>
@artursouza artursouza merged commit c53fc16 into dapr:master Jan 26, 2024
9 checks passed
@JoshVanL JoshVanL added this to the v1.13 milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants