-
Notifications
You must be signed in to change notification settings - Fork 344
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
Remove unnecessary JOIN #5184
Remove unnecessary JOIN #5184
Conversation
2 birds w/ one stone. i like it! |
seems like more than a |
idk, things seem to have been working fine without this fix for the past year and a half, so I think it's minor. But I don't care what you do to the labels, make it whatever you want |
well ds thresholds have been broken, right? seems more than minor for anyone wanting to use ds thresholds :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Confirmed that the correct values for
TotalTpsThreshold
andTotalKbpsThreshold
show up in- TM's
/api/monitor-config
- API versions 1.1, 2.0, and 3.0 of
/cdns/{name}/configs/monitoring
- TM's
We don't lose any specificity by removing the JOIN, since a Delivery Service (optionally unless using MSO) has only 1 profile.
* Remove unnecessary JOIN * Fix tests, also some context leaks * Add to CHANGELOG (cherry picked from commit 330ac87)
* Remove unnecessary JOIN * Fix tests, also some context leaks * Add to CHANGELOG (cherry picked from commit 330ac87)
What does this PR (Pull Request) do?
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Make a GET request to
/cdn/{{Some CDN}}/configs/monitoring
whereSome CDN
is the name of some CDN containing at least one Delivery Service. Notice the array of Delivery Services is not empty.If this is a bug fix, what versions of Traffic Control are affected?
The following criteria are ALL met by this PR