[dbnode] Fix AggregateQuery limits #3112

arnikola · 2021-01-22T05:43:41Z

No description provided.

arnikola · 2021-01-22T05:44:47Z

src/dbnode/storage/index/aggregate_results_entry_arraypool_gen.go

+type AggregateResultsEntries []AggregateResultsEntry
+
+// Size is the element size of the aggregated result entries.
+func (e AggregateResultsEntries) Size() int {
+	// NB: add 1 to the entries length for each entry's field.
+	length := len(e)
+	for _, entry := range e {
+		length += len(entry.Terms)
+	}
+
+	return length
+}
+


This'll be reworked to not be inside a generated file (also refactor will help getting around having to do the full length calculation per incoming field)

arnikola · 2021-01-22T05:45:08Z

src/dbnode/storage/index/block.go

+		batch       = AggregateResultsEntries(b.opts.AggregateResultsEntryArrayPool().Get())
+		maxBatch    = cap(batch)
+		iterClosed  = false // tracking whether we need to free the iterator at the end.
+		exhaustive  = false


may not need this

yeah seems like opts.exhaustive(size, resultCount) accomplishes the same goal

arnikola · 2021-01-22T05:46:01Z

src/dbnode/storage/index/block.go

 	// update recently queried docs to monitor memory.
 	if results.EnforceLimits() {
-		if err := b.docsLimit.Inc(len(batch), source); err != nil {
+		if err := b.docsLimit.Inc(batch.Size(), source); err != nil {


Will have to make a decision on how docs limit is affected in aggregate results; this is not correct

robskillington · 2021-01-22T06:11:50Z

Ah nice, this will make results fairly deterministic I assume? Can we add integration tests for this? I wonder if we also need limit client side for aggregating results across the nodes in m3/storage.go or not.

wesleyk · 2021-01-22T14:25:54Z

src/dbnode/storage/index/block.go

+		batch       = AggregateResultsEntries(b.opts.AggregateResultsEntryArrayPool().Get())
+		maxBatch    = cap(batch)
+		iterClosed  = false // tracking whether we need to free the iterator at the end.
+		exhaustive  = false


yeah seems like opts.exhaustive(size, resultCount) accomplishes the same goal

wesleyk · 2021-01-22T14:28:08Z

src/dbnode/storage/index/aggregate_results.go

@@ -123,6 +123,7 @@ func (r *aggregatedResults) AggregateResultsOptions() AggregateResultsOptions {

 func (r *aggregatedResults) AddFields(batch []AggregateResultsEntry) (int, int) {
 	r.Lock()
+	maxInsertions := r.aggregateOpts.SizeLimit - r.totalDocsCount


should we be subtracting totalDocsCount or r.resultsMap.size()?

Yeah going to do some refactoring/renaming around this to make it clearer what each limit is; totalDocsCount is not quite correctly calculated at the moment, so will need to make a few touchups around it

wesleyk · 2021-01-22T14:30:57Z

src/dbnode/storage/index/block.go

 				break
 			}

 			field, term := iter.Current()
 			batch = b.appendFieldAndTermToBatch(batch, field, term, iterateTerms)
-			if len(batch) < batchSize {
+			if batch.Size() < maxBatch {


can we add a comment on this check?

wesleyk · 2021-01-22T14:31:34Z

src/dbnode/storage/index/block.go

@@ -714,14 +724,14 @@ func (b *block) aggregateWithSpan(
 	}

 	// Add last batch to results if remaining.


comment could be updated

wesleyk · 2021-01-22T14:32:23Z

src/dbnode/storage/index/aggregate_results.go

+					})
+					valueInsertions++
+				} else {
+					// this value exceeds the limit, so should be released to the underling


…a/fix-limits

…ueries

…a/fix-limits

wesleyk · 2021-01-22T18:05:16Z

src/dbnode/storage/index.go

+	docResults, ok := results.(index.DocumentResults)
+	if !ok { // should never happen
+		state.Lock()
+		err := fmt.Errorf("unknown results type [%T] received during wide query", results)


what makes this invocation a "wide query"?

👍 should be regular query

wesleyk · 2021-01-22T18:06:44Z

src/dbnode/storage/index/aggregate_results.go

@@ -143,159 +140,32 @@ func (r *aggregatedResults) AddFields(batch []AggregateResultsEntry) (int, int)
 		valuesMap := aggValues.Map()
 		for _, t := range entry.Terms {
 			if !valuesMap.Contains(t) {
+				fmt.Println(maxInsertions, valueInsertions, t)


…a/fix-limits

wesleyk

LG with some questions!

wesleyk · 2021-01-22T23:00:35Z

src/dbnode/storage/index/aggregate_results.go

-	valueInsertions := 0
+	defer r.Unlock()
+
+	maxDocs := int(math.MaxInt64)


is this more like, remaining docs we can load? remainingDocs?

wesleyk · 2021-01-22T23:20:22Z

src/dbnode/storage/index/aggregate_results.go

+
+	// NB: already hit doc limit.
+	if maxDocs <= 0 {
+		for idx := 0; idx < len(batch); idx++ {


so since we hit the limit, we essentially have to clean up this entire batch?

Yeah, exactly

wesleyk · 2021-01-22T23:21:06Z

src/dbnode/storage/index/aggregate_results.go

+			limitTripped = true
+		}
+
+		if limitTripped {


can we in-line the check above and remove the limitTripped var?

wesleyk · 2021-01-22T23:22:04Z

src/dbnode/storage/index/aggregate_results.go

-				return err
+		valuesMap := aggValues.Map()
+		for _, t := range entry.Terms {
+			if maxDocs > docs {


if the max is already hit should we break from the for? Or is it a trivial amount of iteration?

We still have to iterate through the remainder here to finalize them

…a/fix-limits

* master: [dbnode] Handle empty slices in convert.FromSeriesIDAndEncodedTags (#3107) [dbnode] Fix AggregateQuery limits (#3112) [coordinator] Marshal return val of kv handler with jsonpb (#3116) [m3db] [m3coordinator] Properly pipe along require-exhaustive param for aggregate queries (#3115)

e3f0282

arnikola added 2 commits January 22, 2021 00:38

[dbnode] AggregateQuery limit fix

476dbd4

Cleanup

5a99682

arnikola commented Jan 22, 2021

View reviewed changes

wesleyk reviewed Jan 22, 2021

View reviewed changes

wesleyk and others added 9 commits January 22, 2021 12:00

[integration test] Add label query limits integration test

a0ca9db

Add paren

5d2173d

Fixed exhaustive case, remove dead code

c4ee6c6

Merge branch 'arnikola/fix-limits' of github.com:m3db/m3 into arnikol…

522543c

…a/fix-limits

Aggregate results changes

ebfaa35

Add proper require exhaustive conversion + integration test for agg q…

849c92b

…ueries

Merge branch 'arnikola/fix-limits' of github.com:m3db/m3 into arnikol…

bd35ca9

…a/fix-limits

Merge branch 'master' into arnikola/fix-limits

cad0e06

Avoid flakiness with high limits

829e6b3

wesleyk reviewed Jan 22, 2021

View reviewed changes

arnikola and others added 5 commits January 22, 2021 14:42

Limit on docs or inserts

31f001b

Fixup integration test

172100f

Merge branch 'arnikola/fix-limits' of github.com:m3db/m3 into arnikol…

2695b90

…a/fix-limits

Add more precise assertions to label query limits integration test

84d5a86

Finish test fixes and refactor

dd5ae2e

arnikola marked this pull request as ready for review January 22, 2021 22:07

arnikola and others added 2 commits January 22, 2021 17:25

Response + lint

b18484e

Improve IT comments

ab538cd

wesleyk approved these changes Jan 22, 2021

View reviewed changes

wesleyk and others added 4 commits January 22, 2021 18:29

Merge branch 'master' into arnikola/fix-limits

505319c

Response + lint

b3fdbba

Fix integrations

4221a99

Merge branch 'arnikola/fix-limits' of github.com:m3db/m3 into arnikol…

704b172

…a/fix-limits

arnikola merged commit e3f0282 into master Jan 23, 2021

arnikola deleted the arnikola/fix-limits branch January 23, 2021 00:26

arnikola added a commit that referenced this pull request Jan 28, 2021

Revert "[dbnode] Fix AggregateQuery limits (#3112)"

5eeaa63

e3f0282

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Fix AggregateQuery limits #3112

[dbnode] Fix AggregateQuery limits #3112

arnikola commented Jan 22, 2021

arnikola Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 22, 2021

robskillington commented Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk left a comment

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

arnikola Jan 23, 2021

		@@ -714,14 +724,14 @@ func (b *block) aggregateWithSpan(
		}

		// Add last batch to results if remaining.

[dbnode] Fix AggregateQuery limits #3112

[dbnode] Fix AggregateQuery limits #3112

Conversation

arnikola commented Jan 22, 2021

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 commented Jan 22, 2021

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

wesleyk left a 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