Skip to content
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

[dbnode] Add aggregate term limit regression test #3135

Merged
merged 4 commits into from
Jan 29, 2021

Conversation

arnikola
Copy link
Collaborator

Adds a regression test to catch the error seen in #3133; also exercises a test case that caused issues on that path (batch limit was unbounded as long as terms remained the same, which caused issues allocing unbounded slices of terms per block of aggregate query)

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #3135 (892cdd6) into master (922a33b) will increase coverage by 1.4%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3135     +/-   ##
=========================================
+ Coverage    70.8%    72.3%   +1.4%     
=========================================
  Files        1083     1084      +1     
  Lines      100219   100236     +17     
=========================================
+ Hits        71005    72475   +1470     
+ Misses      24139    22726   -1413     
+ Partials     5075     5035     -40     
Flag Coverage Δ
aggregator 75.8% <ø> (+<0.1%) ⬆️
cluster 84.8% <ø> (ø)
collector 84.3% <ø> (ø)
dbnode 78.7% <100.0%> (+3.4%) ⬆️
m3em 74.4% <ø> (ø)
m3ninx 73.2% <ø> (+<0.1%) ⬆️
metrics 20.0% <ø> (ø)
msg 73.7% <ø> (-0.5%) ⬇️
query 67.2% <ø> (ø)
x 80.6% <ø> (+0.2%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.


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 922a33b...384d8a1. Read the comment docs.

defer ctx.BlockingClose()

// create initial span from a mock tracer and get ctx
ctx.SetGoContext(opentracing.ContextWithSpan(stdlibctx.Background(), nil))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah interesting, why do we need to create a span for the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah damn good catch, thought I'd removed this, the test I used as a base did some verification on tracepoints

@arnikola arnikola merged commit bef2564 into master Jan 29, 2021
@arnikola arnikola deleted the arnikola/doc-prop-test branch January 29, 2021 14:55
soundvibe added a commit that referenced this pull request Jan 29, 2021
* master:
  [dbnode] Add aggregate term limit regression test (#3135)
  [DOCS] Adding Prometheus steps to quickstart (#3043)
  [dbnode] Revert AggregateQuery changes (#3133)
  Fix TestSessionFetchIDs flaky test (#3132)
  [dbnode] Alter multi-segments builder to order by size before processing (#3128)
  [dbnode] Emit aggregate usage metrics (#3123)
  [dbnode] Add Shard.OpenStreamingReader method (#3119)
  [dtests] Docker tests integration with docker-compose (#3031)
  [dbnode] Comments / remove unused var (#3124)
  [query] Handle context.Canceled and map to 499 http status (#3069)
  [dbnode] Use StreamingReadMetadata for bootstrapping (#2938)
  [dbnode] Use DefaultTestOptions in test code (#3113)

# Conflicts:
#	src/dbnode/storage/bootstrap/bootstrapper/fs/source.go
soundvibe added a commit that referenced this pull request Feb 1, 2021
* master:
  [dtest] endpoint to fetch tagged (#3138)
  Refactor FetchTagged to return an Iterator of results (#3141)
  [dbnode] Add aggregate term limit regression test (#3135)
  [DOCS] Adding Prometheus steps to quickstart (#3043)
  [dbnode] Revert AggregateQuery changes (#3133)
  Fix TestSessionFetchIDs flaky test (#3132)
  [dbnode] Alter multi-segments builder to order by size before processing (#3128)
  [dbnode] Emit aggregate usage metrics (#3123)
  [dbnode] Add Shard.OpenStreamingReader method (#3119)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants