[query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules #2782
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
rallen090
reviewed
Oct 22, 2020
rallen090
reviewed
Oct 22, 2020
rallen090
approved these changes
Oct 22, 2020
arnikola
reviewed
Oct 23, 2020
arnikola
reviewed
Oct 23, 2020
nbroyles
force-pushed
the
nb/use-staging-state
branch
from
94c4b0a to
9115e64
Compare
Codecov Report
@@ Coverage Diff @@
## nb/use-staging-state #2782 +/- ##
=======================================================
+ Coverage 45.6% 50.7% +5.1%
=======================================================
Files 4 1013 +1009
Lines 274 92663 +92389
=======================================================
+ Hits 125 47061 +46936
- Misses 139 41554 +41415
- Partials 10 4048 +4038
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
arnikola
reviewed
Oct 23, 2020
arnikola
reviewed
Oct 23, 2020
arnikola
reviewed
Oct 23, 2020
arnikola
reviewed
Oct 23, 2020
arnikola
reviewed
Oct 26, 2020
Comment on lines
+162
to
+182
| defaultStagedMetadatasProtos := make([]metricpb.StagedMetadatas, 0, len(autoMappingRules)) | ||
| for _, rule := range autoMappingRules { | ||
| metadatas, err := rule.StagedMetadatas() | ||
| if err != nil { | ||
| logger.Error("could not generate staged metadata from automapping rules."+ | ||
| " aggregations will continue with current configuration.", zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| var metadatasProto metricpb.StagedMetadatas | ||
| if err := metadatas.ToProto(&metadatasProto); err != nil { | ||
| logger.Error("could not generate staged metadata from automapping rules."+ | ||
| " aggregations will continue with current configuration.", zap.Error(err)) | ||
| return | ||
| } | ||
|
|
||
| defaultStagedMetadatasProtos = append(defaultStagedMetadatasProtos, metadatasProto) | ||
| } | ||
|
|
||
| d.Lock() | ||
| d.metricsAppenderOpts.defaultStagedMetadatasProtos = defaultStagedMetadatasProtos |
There was a problem hiding this comment.
nit: it may be better to do this whole section under d.Lock() so that you can mutate d.metricsAppenderOpts.defaultStagedMetadatasProtos directly without needing the alloc on 162?
There was a problem hiding this comment.
Ah, I think that only works if you end up with the same number of automapping rules after the update -- which is probably a corner case. Most likely, you'll have more automappings rules than before (as a result of a namespace addition) meaning we'd need re-size the existing one.
arnikola
reviewed
Oct 26, 2020
arnikola
reviewed
Oct 26, 2020
arnikola
reviewed
Oct 26, 2020
nbroyles
force-pushed
the
nb/use-staging-state
branch
from
9115e64 to
8720ea2
Compare
arnikola
reviewed
Oct 26, 2020
arnikola
reviewed
Oct 26, 2020
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:
With the change to make the coordinator resolve namespaces dynamically, parts of the coordinator that expect to have all available namespaces on startup need to be updated. This PR adds a cluster namespaces watcher that parts of the coordinator can use to get notified of changes. It also updates the downsampler to make use of this watcher to generate automapping rules.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: