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

plan: remove mutexes #7468

Merged
merged 1 commit into from
Feb 9, 2021
Merged

Conversation

vmg
Copy link
Collaborator

@vmg vmg commented Feb 8, 2021

Description

The mutexes that were guarding the 'stats' section of plans in vtgate
and vttablet were not being properly obeyed. A lot of the code was
accessing the stats without acquiring the mutex first, mostly because
there is no global consistency requirement between all the fields of the
stats. Because of this, remove the Mutexes from both plans and switch to
using Atomic operations to handle the stats fields. This reduces the
sizes of the plans on cache and improves the accuracy of our memory
usage metrics, since we do not have accurate size calculations for mutex
objects, as they're foreign to the project.

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

The mutexes that were guarding the 'stats' section of plans in vtgate
and vttablet were not being properly obeyed. A lot of the code was
accessing the stats without acquiring the mutex first, mostly because
there is no global consistency requirement between all the fields of the
stats. Because of this, remove the Mutexes from both plans and switch to
using Atomic operations to handle the stats fields. This reduces the
sizes of the plans on cache and improves the accuracy of our memory
usage metrics, since we do not have accurate size calculations for mutex
objects, as they're foreign to the project.

Signed-off-by: Vicent Marti <vmg@strn.cat>
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

With this change there is no consistency between all the plan stats. May be that is not important. Just wanted to call that out.

@vmg
Copy link
Collaborator Author

vmg commented Feb 9, 2021

@harshit-gangal: yes, that's explained in the commit message:

A lot of the code was accessing the stats without acquiring the mutex first, mostly because there is no global consistency requirement between all the fields of the stats

In practice, there was no consistency between plan stats before either, because most of the code that was reading the stats was doing so without acquiring the mutex first. That's why I thought this would be a good idea.

@harshit-gangal harshit-gangal merged commit 2002d79 into vitessio:master Feb 9, 2021
@askdba askdba added this to the v10.0 milestone Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants