[dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease #2790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
notbdu
reviewed
Oct 27, 2020
| isUpdatingLease bool | ||
| status seekerManagerStatus | ||
| shardSet sharding.ShardSet | ||
| updateOpenLeasesInProgress map[block.HashableLeaseDescriptor]struct{} |
There was a problem hiding this comment.
+1 on consistent naming w/ what's in block/lease.go
notbdu
reviewed
Oct 27, 2020
| func (m *seekerManager) startUpdateOpenLease( | ||
| namespace ident.ID, | ||
| hashableDescriptor block.HashableLeaseDescriptor, | ||
| ) (bool, error) { | ||
| m.Lock() | ||
| defer m.Unlock() |
There was a problem hiding this comment.
nit: would we want to release this lock and perhaps use a sync.Map like we do in block/lease.go for tracking open leases? Although tbh perf wise this prob wouldn't make a diff at all.
There was a problem hiding this comment.
Using sync.Map (as I did in block/lease.go) was the initial approach that I tried, but then I realized that the lock is being held for the whole duration of startUpdateOpenLease, and there is this namespace check in between progress check / update, so I decided to go with a simple map under this lock (to better preserve the original semantics).
linasm
deleted the
linasm/fix-seekerManager-UpdateOpenLease-granularity
branch
soundvibe
added a commit
that referenced
this pull request
Oct 29, 2020
* master: Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) # Conflicts: # src/query/parser/promql/matchers.go
soundvibe
added a commit
that referenced
this pull request
Oct 29, 2020
* master: [query] Improve precision for variance and stddev of equal values (#2799) Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) [query] Add "median" aggregation to Graphite aggregate() function (#2774) [r2] Ensure KeepOriginal is propagated when adding/reviving rules (#2796) [dbnode] Update default db read limits (#2784)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
seekerManager.UpdateOpenLease(...)was preventing concurrent calls at global granularity (see the removed comment). This changes it to the smallest granularity possible (namespace + shard + block start) to allow concurrent operations.Such change would prevent possible collisions between
shard.AggregateTiles()background running and manual invocations, andshardColdFlush.Done().Special notes for your reviewer:
Noticed this while investigating a flaky integration test.
Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE
Does this PR require updating code package or user-facing documentation?:
NONE