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

[query] Add determinism to Graphite sorting and reduce functions #3164

Merged
merged 2 commits into from
Feb 4, 2021

Conversation

robskillington
Copy link
Collaborator

@robskillington robskillington commented Feb 3, 2021

What this PR does / why we need it:

Fixes the Graphite sorting and reduce functions to be deterministic if there is multiple ranked series at the same sort order. This is done by using sort.Stable and if there was no incoming sort order into the function then applying sort by name to have determinism on which sort.Stable will pick as results.

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

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #3164 (f422699) into master (a7a0580) will decrease coverage by 32.2%.
The diff coverage is 39.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3164      +/-   ##
=========================================
- Coverage    72.4%   40.1%   -32.3%     
=========================================
  Files        1085     993      -92     
  Lines      101127   92688    -8439     
=========================================
- Hits        73233   37190   -36043     
- Misses      22839   51901   +29062     
+ Partials     5055    3597    -1458     
Flag Coverage Δ
aggregator 52.4% <ø> (-23.5%) ⬇️
cluster 36.2% <ø> (-48.6%) ⬇️
collector 73.3% <ø> (-11.0%) ⬇️
dbnode 43.2% <ø> (-35.7%) ⬇️
m3em 50.2% <ø> (-24.2%) ⬇️
m3ninx 49.5% <ø> (-23.7%) ⬇️
metrics ?
msg 73.2% <ø> (-0.9%) ⬇️
query 22.0% <39.4%> (-45.2%) ⬇️
x 46.8% <ø> (-33.8%) ⬇️

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 a7a0580...f422699. Read the comment docs.

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

LGTM

@robskillington robskillington merged commit 985f866 into master Feb 4, 2021
@robskillington robskillington deleted the r/fix-graphite-limit-lowest-not-ordered branch February 4, 2021 03:47
soundvibe added a commit that referenced this pull request Feb 4, 2021
* master:
  [query] Add determinism to Graphite sorting and reduce functions (#3164)
  [docs] Update footer (#3165)
  Return an Iterator from ReadEncoded (#3147)
  [changelog] Add changelog for 1.1.0 (#3160)
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