Skip to content

Fix default metrics_resolution language #4413

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

Closed
wants to merge 2 commits into from
Closed

Fix default metrics_resolution language #4413

wants to merge 2 commits into from

Conversation

lpsantil
Copy link

PR for #4412

@ahardin-rh
Copy link
Contributor

@lpsantil Thank you! These changes look good to me. Can you please squash this into one commit, however? That will help us with seamless cherry-picking and publishing. Thanks again!

@ahardin-rh
Copy link
Contributor

@lpsantil Actually, I just noticed that this PR is submitted against enterprise-3.5. Can you please submit it against the master branch, then we can cherry-pick it to the other relevant branches like 3.5? Thanks! Sorry for any confusion!

@lpsantil
Copy link
Author

@ahardin-rh @bfallonf Would you mind reviewing #4411 before I push this stuff to master?

@bfallonf
Copy link

@lpsantil I'm trying to figure out the deal with that PR right now. I think different versions having different capacities might impact what happens there.

That said, I think this PR can merge without affecting that one, so feel free to move ahead with this.

@ahardin-rh
Copy link
Contributor

Looks like this is still not resolved. I think we should check with @jeremyeder if this guidance is correct for OCP 3.5 and, if so, if this guidance should be updated for other OCP versions as well. If not, we may not need to submit this against master.

@vikram-redhat
Copy link
Contributor

@ahardin-rh @lpsantil - I have replied in #4411 .

@ncbaratta ncbaratta added the peer-review-needed Signifies that the peer review team needs to review this PR label Nov 9, 2017
@vikram-redhat vikram-redhat modified the milestones: Next Release, Staging Jan 8, 2018
Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

@ahardin-rh, is this PR still valid?

@@ -185,8 +185,8 @@ These two test cases are presented on the following graph:
image::https://raw.githubusercontent.com/ekuric/openshift/master/metrics/1_10kpods.png[1000 pods vs 10000 pods monitored during 24 hours]
endif::openshift-origin[]

If the default value of 7 days for `openshift_metrics_duration` and 10 seconds for
`openshift_metrics_resolution` are preserved, then weekly storage requirements for the Cassandra pod would be:
If the default value of 7 days for `openshift_metrics_duration` is preserved, and
Copy link
Contributor

Choose a reason for hiding this comment

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

s/preserved,/preserved

If the default value of 7 days for `openshift_metrics_duration` and 10 seconds for
`openshift_metrics_resolution` are preserved, then weekly storage requirements for the Cassandra pod would be:
If the default value of 7 days for `openshift_metrics_duration` is preserved, and
`openshift_metrics_resolution` is set to 10 seconds, then weekly storage requirements for the Cassandra pod would be:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/would be/are

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 27, 2018
@ahardin-rh
Copy link
Contributor

@kalexand-rh I'm not sure. I think this is all related to #4411, where @vikram-redhat commented. 🤷‍♀️

@vikram-redhat
Copy link
Contributor

I am going to close this PR and the associated issue since we never got confirmation from the metrics team.

Please feel free to open a new PR/issue if this still needs updating (and perhaps for newer versions of OCP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-3.5 branch/enterprise-3.6 peer-review-done Signifies that the peer review team has reviewed this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants