-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
chore: sync tables without acquiring read lock the whole time #14179
Conversation
5879eff
to
b3e609d
Compare
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.
lgtm, just one question. Is it still safe to call indexSet.Sync
or table.Sync
without holding a lock over t.indexSetMtx
or tm.tablesMtx
respectively? That's the only thing that poped into my head while reviewing
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.
lgtm but I haven't tested it
Thanks @JoaoBraveCoding! I have been running these change in our dev and ops env, no issues so far |
What this PR does / why we need it:
Table manager and Table grab a read lock for the entire duration when syncing tables and index sets respectively, this can prevent new tables and new indexsets from getting initialised while the rlock is held. This pr tries to reduce the possibility of sync calls affecting query time table and indexset initialisation by grabbing the lock only for accessing protected resource during the sync loop
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR