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

[m3db] [m3coordinator] Properly pipe along require-exhaustive param for aggregate queries #3115

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

wesleyk
Copy link
Collaborator

@wesleyk wesleyk commented Jan 22, 2021

What this PR does / why we need it:
[m3db] [m3coordinator] Properly pipe along require-exhaustive param for aggregate queries

Special notes for your reviewer:

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

@@ -432,6 +433,7 @@ struct AggregateQueryRequest {
8: optional TimeType rangeType = TimeType.UNIX_SECONDS
9: optional binary source
10: optional i64 docsLimit
11: optional bool requireExhaustive
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this indicate that the results are exhaustive or whether you passed the flag in in the first place? If it's the former, let's just call it exhaustive

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's on the request side, saying it requires the results to be exhaustive. Would rather stick with that as it's how our other requests use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, my bad, this is a request. Not a result. Misread the struct

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@wesleyk wesleyk merged commit c74143f into master Jan 22, 2021
@wesleyk wesleyk deleted the wesley-split branch January 22, 2021 19:23
soundvibe added a commit that referenced this pull request Jan 25, 2021
* 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)
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.

3 participants