[query] Fanout options to select namespaces #1328

arnikola · 2019-01-30T01:38:54Z

No description provided.

codecov · 2019-01-30T01:51:19Z

Codecov Report

Merging #1328 into master will decrease coverage by 0.2%.
The diff coverage is 83.1%.

@@           Coverage Diff            @@
##           master   #1328     +/-   ##
========================================
- Coverage    70.7%   70.4%   -0.3%     
========================================
  Files         814     811      -3     
  Lines       69314   69008    -306     
========================================
- Hits        49005   48647    -358     
- Misses      17181   17237     +56     
+ Partials     3128    3124      -4

Flag	Coverage Δ
#aggregator	`80.9% <ø> (-0.6%)`	⬇️
#cluster	`85.6% <ø> (ø)`	⬆️
#collector	`78.4% <ø> (ø)`	⬆️
#dbnode	`80.6% <ø> (-0.4%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.7% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.8% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (-0.2%)`	⬇️
#query	`64% <83.1%> (-0.4%)`	⬇️
#x	`75.9% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c85f85...fbe2219. Read the comment docs.

codecov · 2019-01-30T01:51:23Z

Codecov Report

Merging #1328 into master will increase coverage by <.1%.
The diff coverage is 81.2%.

@@           Coverage Diff            @@
##           master   #1328     +/-   ##
========================================
+ Coverage    70.5%   70.6%   +<.1%     
========================================
  Files         816     815      -1     
  Lines       69452   69363     -89     
========================================
- Hits        49033   49006     -27     
+ Misses      17291   17228     -63     
- Partials     3128    3129      +1

Flag	Coverage Δ
#aggregator	`81.4% <ø> (ø)`	⬆️
#cluster	`85.6% <ø> (ø)`	⬆️
#collector	`78.4% <ø> (ø)`	⬆️
#dbnode	`80.9% <ø> (-0.1%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.7% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.8% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (-0.2%)`	⬇️
#query	`64.2% <81.2%> (+0.2%)`	⬆️
#x	`75.9% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c85f85...077d2c6. Read the comment docs.

robskillington · 2019-01-30T13:21:10Z

src/query/graphite/storage/m3_wrapper.go

+	fetchOptions.FanoutOptions = &storage.FanoutOptions{
+		FanoutUnaggregated:        storage.FanoutForceDisable,
+		FanoutAggregated:          storage.FanoutForceEnable,
+		FanoutAggregatedOptimized: storage.FanoutForceEnable,


I think we want this to be FanoutForceDisable for the optimized flag so that all aggregated namespaces look like "partial" namespaces?

robskillington · 2019-01-30T14:32:37Z

src/query/storage/m3/storage.go

+		unaggregated ClusterNamespace
+		retention    time.Duration
+		enabled      bool
+	}


Maybe move this type to top of this file?

arnikola · 2019-01-30T21:50:02Z

scripts/docker-integration-tests/carbon/test.sh

@@ -57,5 +57,3 @@ t=$(date +%s)
 echo "foo.min.catch-all.baz 10 $t" | nc 0.0.0.0 7204
 echo "foo.min.catch-all.baz 20 $t" | nc 0.0.0.0 7204
 echo "Attempting to read mean aggregated carbon metric"
-ATTEMPTS=10 TIMEOUT=1 retry_with_backoff read_carbon foo.min.catch-all.baz 15


Accidentally deleted

robskillington · 2019-01-31T02:30:52Z

src/query/storage/m3/cluster_resolver.go

+	if unaggregatedStart.Before(start) || unaggregatedStart.Equal(start) {
+		return unaggregatedNamespaceDetails{
+			clusterNamespace: unaggregated,
+			satisfies:        fullySatisfiesRange,


nit: Is it worth including retention in return for this code path? In case it's needed in future and we don't find out that it's set to zero with tests or something?

Sure, sounds good.

robskillington · 2019-01-31T02:31:50Z

src/query/storage/m3/cluster_resolver.go

+	//
+	// NB: if fanout aggregation is forced on, the filter instead forces clusters
+	// that do not cover the range to be set as partially aggregated.
+	filterFunc := func(namespace ClusterNamespace) bool {


nit: Maybe we call this coversRangeFilter to declare intent of the filter in later code?

robskillington · 2019-01-31T02:36:33Z

src/query/graphite/storage/m3_wrapper.go

+	fetchOptions := storage.NewFetchOptions()
+	fetchOptions.FanoutOptions = &storage.FanoutOptions{
+		FanoutUnaggregated:        storage.FanoutForceDisable,
+		FanoutAggregated:          storage.FanoutForceEnable,


Hm, should we specifying Default perhaps here?

robskillington · 2019-01-31T02:39:38Z

src/query/storage/m3/cluster_resolver.go

+
+	// No need to regenerate aggregated namespace without a filter if aggregated
+	// fanout is force enabled.
+	if opts.FanoutAggregated != storage.FanoutForceEnable {


Hm isn't the default to enable fanning out to aggregated namespaces in general?

i.e. if ForceEnable and Default should both always return aggregated namespaces, and for FanoutAggregatedOptimized that should just determine whether to include or not include namespaces with downsampleOpts.All into the complete or partial bucket.

So I would've thought for opts.FanoutAggregated == FanoutForceDisable we would just always return the unaggregated namespace, and no namespaces if there is no unaggregated namespace at all. And then we shouldn't check this flag any later I would've thought?

robskillington · 2019-01-31T02:41:52Z

src/query/storage/m3/cluster_resolver.go

+		result := r.partialAggregated
+		// If unaggregated namespace can partially satisfy this range, add it as a
+		// fanout contender.
+		if unaggregated.satisfies == partiallySatisfiesRange {


Is this check necessary? If it fully satisifies the query range it would've already be returned on line 87 as part of the first check no?

Yeah, it's to check that it's not disabled here... I didn't want to use two bools like enabled, coversRange bool since they're kinda linked

robskillington · 2019-01-31T02:43:19Z

src/query/storage/m3/cluster_resolver.go

+		// range, set query fanout type to namespaceCoversPartialQueryRange.
+		for _, n := range result {
+			clusterStart := now.Add(-1 * n.Options().Attributes().Retention)
+			if clusterStart.After(start) {


Unfortunately you need to do clusterStart.Equal(..) || clusterStart.After(...) to take into account the case where the start and cluster start is an exact match.

Ah damn I thought I flipped it to !clusterStart.Before(..)

robskillington · 2019-01-31T02:46:49Z

src/query/storage/m3/cluster_resolver.go

+	slices = slices.reset(len(all))
+
+	// Force disable fanout to any aggregated namespaces.
+	if opts.FanoutAggregated == storage.FanoutForceDisable {


As discussed before, if FanoutAggregated is actually disabled, then we can just probably return immediately after checking the unaggregated namespace. (i.e. return unaggregated as long as that is default or enabled) and then don't need to check it again at all.

Yeah, it gets to the same conclusion but with more steps; will try and short circuit it instead

robskillington · 2019-01-31T03:11:34Z

src/query/storage/m3/cluster_resolver.go

+		}
+
+		// If fanout is forced, ignore the filter.
+		if opts.FanoutAggregated != storage.FanoutForceEnable {


I was thinking we would not actually need to skip the check here, since if FanoutAggregatedOptimized is forced disabled, then you'll always run this function with filter == nil because you won't take the path of if len(r.completeAggregated) > 0 after running the first call to aggregatedNamespaces(...).

robskillington · 2019-01-31T14:38:40Z

src/query/storage/m3/storage.go

@@ -52,6 +53,7 @@ type queryFanoutType uint
 const (
 	namespaceCoversAllQueryRange queryFanoutType = iota
 	namespaceCoversPartialQueryRange
+	namespaceInvalid


nit: Maybe make this the first value in the enum?

That may cause errors later which would be hard to track? This is more used as just an invalid 'nil'ish value, still makes more sense to put it as first value?

robskillington · 2019-01-31T15:10:15Z

src/query/storage/m3/cluster_resolver.go

+
+	if opts.FanoutAggregated == storage.FanoutForceDisable {
+		if unaggregated.satisfies == partiallySatisfiesRange {
+			return namespaceCoversAllQueryRange,


nit: Probably better to return namespaceCoversPartialQueryRange here since it only partially satisfies the range?

robskillington

LGTM

Rob Skillington and others added 4 commits January 29, 2019 10:28

Add changes to support fanout control for graphite

1d4d844

Plumbs fanout control down to graphite query handler

ed332f6

Linter fixes

148e9ce

Tests for cluster fanout

8fa62a3

arnikola changed the title ~~R/add fanout option~~ [query] Fanout options to select namespaces Jan 30, 2019

Merge branch 'master' into r/add-fanout-option

fbe2219

robskillington reviewed Jan 30, 2019

View reviewed changes

arnikola added 3 commits January 30, 2019 16:11

WIP, cleanup required

fa79ff0

Refactor to cluster resolver approach

343898f

Revert changes

a7133a1

arnikola commented Jan 30, 2019

View reviewed changes

Cleanup

9567ae0

robskillington reviewed Jan 31, 2019

View reviewed changes

PR response and collapsing some conditional paths

a2d0839

robskillington reviewed Jan 31, 2019

View reviewed changes

robskillington approved these changes Jan 31, 2019

View reviewed changes

Nit responses

077d2c6

arnikola merged commit 806e93b into master Jan 31, 2019

arnikola deleted the r/add-fanout-option branch January 31, 2019 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Fanout options to select namespaces #1328

[query] Fanout options to select namespaces #1328

arnikola commented Jan 30, 2019

codecov bot commented Jan 30, 2019

codecov bot commented Jan 30, 2019 •

edited

Loading

robskillington Jan 30, 2019 •

edited

Loading

robskillington Jan 30, 2019

arnikola Jan 30, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington Jan 31, 2019 •

edited

Loading

arnikola Jan 31, 2019

robskillington Jan 31, 2019

robskillington Jan 31, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington Jan 31, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington Jan 31, 2019

arnikola Jan 31, 2019

robskillington left a comment

[query] Fanout options to select namespaces #1328

[query] Fanout options to select namespaces #1328

Conversation

arnikola commented Jan 30, 2019

codecov bot commented Jan 30, 2019

Codecov Report

codecov bot commented Jan 30, 2019 • edited Loading

Codecov Report

robskillington Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2019 •

edited

Loading

robskillington Jan 30, 2019 •

edited

Loading

robskillington Jan 31, 2019 •

edited

Loading