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.

Sign up for GitHub

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

Jump to bottom

[query] Add cost enforcer to graphite queries #1449

Merged
merged 5 commits into from
Mar 13, 2019
Merged

[query] Add cost enforcer to graphite queries #1449

merged 5 commits into from
Mar 13, 2019

Conversation

arnikola
Copy link
Collaborator

No description provided.

@andrewmains12 andrewmains12 mentioned this pull request Mar 13, 2019
Closed
andrewmains12
andrewmains12 reviewed Mar 13, 2019
View reviewed changes
src/query/storage/converter.go Outdated

func (e *syncEnforcer) Add(c xcost.Cost) xcost.Report {
e.mu.Lock()
r := e.enforcer.Add(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary, I don't think; the enforcers are designed to be thread safe (through use of atomic variables). If they're not, we should probably fix it at that level instead of adding a wrapper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, no worries, was probably being a little overcautious here

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #1449 into andrewmains12/cost_accounting will decrease coverage by <.1%.
The diff coverage is 63.8%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           andrewmains12/cost_accounting   #1449     +/-   ##
===============================================================
- Coverage                           70.9%   70.9%   -0.1%     
===============================================================
  Files                                840     840             
  Lines                              71753   71779     +26     
===============================================================
+ Hits                               50918   50934     +16     
- Misses                             17506   17521     +15     
+ Partials                            3329    3324      -5
Flag Coverage Δ
#aggregator 82.3% <ø> (ø) ⬆️
#cluster 85.8% <ø> (ø) ⬆️
#collector 63.7% <ø> (ø) ⬆️
#dbnode 80.8% <ø> (ø) ⬆️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.2% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.6% <ø> (ø) ⬆️
#msg 74.9% <ø> (-0.2%) ⬇️
#query 66.1% <63.8%> (-0.1%) ⬇️
#x 76.3% <ø> (ø) ⬆️

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 f4d13bc...87cce05. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 13, 2019 •
edited
Loading

Codecov Report

Merging #1449 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1449   +/-   ##
======================================
  Coverage    70.9%   70.9%           
======================================
  Files         840     840           
  Lines       71753   71753           
======================================
  Hits        50917   50917           
  Misses      17512   17512           
  Partials     3324    3324
Flag Coverage Δ
#aggregator 82.3% <0%> (ø) ⬆️
#cluster 85.8% <0%> (ø) ⬆️
#collector 63.7% <0%> (ø) ⬆️
#dbnode 80.8% <0%> (ø) ⬆️
#m3em 73.2% <0%> (ø) ⬆️
#m3ninx 74.2% <0%> (ø) ⬆️
#m3nsch 51.1% <0%> (ø) ⬆️
#metrics 17.6% <0%> (ø) ⬆️
#msg 74.9% <0%> (ø) ⬆️
#query 66.2% <0%> (ø) ⬆️
#x 76.3% <0%> (ø) ⬆️

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 b1213c2...8dbae9f. Read the comment docs.

andrewmains12
andrewmains12 approved these changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@andrewmains12 andrewmains12 left a comment

Choose a reason for hiding this comment

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

Lgtm! Winner of the fastest PR award :)

@arnikola arnikola mentioned this pull request Mar 13, 2019
Closed
@andrewmains12 andrewmains12 force-pushed the andrewmains12/cost_accounting branch from f4d13bc to 509bee5 Compare March 13, 2019 17:55
@arnikola arnikola changed the base branch from andrewmains12/cost_accounting to master March 13, 2019 18:50
arnikola added 2 commits March 13, 2019 14:57
@arnikola
Merge branch 'master' into arnikola/graphite-cost-accounting
8dbae9f 
# Conflicts:
#	src/query/api/v1/httpd/handler_test.go
#	src/query/storage/converter.go
#	src/query/storage/converter_test.go
@arnikola
Fix bad merge in test
fc7a334
@arnikola arnikola merged commit 4610505 into master Mar 13, 2019
@arnikola arnikola deleted the arnikola/graphite-cost-accounting branch March 13, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewmains12 andrewmains12 andrewmains12 approved these changes

@benraskin92 benraskin92 Awaiting requested review from benraskin92

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arnikola @andrewmains12