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

TM should not overwrite monitoring snapshot data with crconfig snapsot data #6443

Merged
merged 3 commits into from
Jan 7, 2022
Merged

TM should not overwrite monitoring snapshot data with crconfig snapsot data #6443

merged 3 commits into from
Jan 7, 2022

Conversation

srijeet0406
Copy link
Contributor

This PR closes #6376

Which Traffic Control components are affected by this PR?

  • Traffic Monitor
  • Traffic Ops

What is the best way to verify this PR?

Make sure all the tests run. Follow the steps in #6376 and make sure you see the expected results.

If this is a bugfix, which Traffic Control versions contained the bug?

  • master

PR submission checklist

@zrhoffman zrhoffman added improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops labels Jan 3, 2022
tcfdev
tcfdev suggested changes Jan 3, 2022
View reviewed changes
Copy link
Collaborator

@tcfdev tcfdev left a comment

Choose a reason for hiding this comment

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

First round :)

traffic_monitor/towrap/towrap.go Outdated Show resolved Hide resolved
traffic_monitor/towrap/towrap.go Outdated Show resolved Hide resolved
traffic_monitor/towrap/towrap.go Outdated Show resolved Hide resolved
traffic_monitor/towrap/towrap.go Show resolved Hide resolved
traffic_ops/traffic_ops_golang/monitoring/monitoring.go Show resolved Hide resolved
traffic_ops/traffic_ops_golang/monitoring/monitoring.go Outdated Show resolved Hide resolved
traffic_ops/traffic_ops_golang/monitoring/monitoring_test.go Outdated Show resolved Hide resolved
tcfdev
tcfdev approved these changes Jan 6, 2022
View reviewed changes
Copy link
Collaborator

@tcfdev tcfdev left a comment

Choose a reason for hiding this comment

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

One last code comment clean and then it LGTM! Ran it briefly in a test environment and all appears to be working as intended.

traffic_monitor/towrap/towrap.go Outdated Show resolved Hide resolved
@shamrickus shamrickus merged commit d5ba824 into apache:master Jan 7, 2022
@rawlinp rawlinp mentioned this pull request Jan 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcfdev tcfdev tcfdev approved these changes

Assignees
No one assigned
Labels
improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TM should not need to overwrite monitoring snapshot data with CRConfig snapshot data
4 participants
@srijeet0406 @tcfdev @shamrickus @zrhoffman