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

Multiple runs on scale-from-zero test #3059

Conversation

greghaynes
Copy link
Contributor

This is a fairly noisy test, lets do 5 runs and report a min/max/avg so
we can get more clear data.

Release Note

NONE

@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 1, 2019
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@greghaynes: 0 warnings.

In response to this:

This is a fairly noisy test, lets do 5 runs and report a min/max/avg so
we can get more clear data.

Release Note

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@srinivashegde86
Copy link
Contributor

srinivashegde86 commented Feb 1, 2019

we have the fortio integration to send load. If you can use that, it provides all these stats already.
https://github.com/knative/serving/tree/master/test/performance#load-generator

var avg time.Duration
var avg, min, max time.Duration
min = durations[0]
max = durations[0]

for _, dur := range durations {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for _, dur := range durations {
for _, dur := range durations[1:] {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

although the min/max checks are a noop on the first loop, I need to run it for the avg aggregating.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but you can also do avg=durations[0], no? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, was optimizing for LOC :)

@adrcunha
Copy link
Contributor

adrcunha commented Mar 7, 2019

/uncc @adrcunha
/assign @srinivashegde86

@knative-prow-robot knative-prow-robot removed the request for review from adrcunha March 7, 2019 18:58
@adrcunha adrcunha requested a review from srinivashegde86 March 7, 2019 18:59
@greghaynes greghaynes force-pushed the feature/scale-from-zero-multirun branch from 3830126 to 2ef0f44 Compare April 1, 2019 20:56
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 1, 2019
@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Apr 1, 2019
@greghaynes greghaynes force-pushed the feature/scale-from-zero-multirun branch 2 times, most recently from d6e51d6 to b2b4cca Compare April 1, 2019 21:50
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

Stylistic nit, otherwise looks good.

This is a fairly noisy test, lets do 5 runs and report a min/max/avg so
we can get more clear data.
@greghaynes greghaynes force-pushed the feature/scale-from-zero-multirun branch from b2b4cca to 6905b32 Compare April 1, 2019 22:51
@srinivashegde86
Copy link
Contributor

/retest

@srinivashegde86
Copy link
Contributor

/approve

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2019
@srinivashegde86
Copy link
Contributor

/lgtm
/hold
if @vagababov wants to take a look.

@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 2, 2019
@greghaynes
Copy link
Contributor Author

@vagababov Could you PTAL? Thanks!

Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: greghaynes, srinivashegde86, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 3f85158 into knative:master Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants