Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

Conversation

@marcemq
Copy link
Contributor

@marcemq marcemq commented Nov 22, 2019

No description provided.

Description:
Launch a series of workloads and take memory metric measurements after
each launch.
Launch a series of workloads and take pod query-response latency measurements
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO query-response can be redundant, latency in most of the cases refers to the time that something takes to response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken this description from the ongoing PR212 of such test.

@askervin any thoughts on this?

Choose a reason for hiding this comment

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

I think query-response is fine (although you may wish to add 'http' or whatever protocol is being used to it). Without saying query-response, then I don't think we'd know which latency was being measured (network, boot, etc.).

Description:
Launch a series of workloads and take memory metric measurements after
each launch.
Launch a series of workloads and take memory metric measurements using collectd
Copy link
Contributor

Choose a reason for hiding this comment

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

a little suggestion: using single quotes to refer another command/application e.g. collectd to avoid confusion with a typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Morales Quispe, Marcela <marcela.morales.quispe@intel.com>
@askervin
Copy link
Contributor

askervin commented Nov 26, 2019

LGTM.

PR #212 changes the network latency test script name from test_scale_nc.sh to test_rapid_nc.sh, as it is based on the rapid/collectd test now - instead of the original grab_stats. Having this change merged will remind me (in the form of a merge conflict) that PR #212 should change the script documentation as well.

@grahamwhaley
Copy link

No description provided.

Please provide meaninful descriptions in the PR and the commit bodies, even if they are brief and mostly are repeat of the subject - having a 'what and why and how' in the git logs helps in the future.

Copy link

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

thanks for cleaning these up. Just a couple of minor nits.

Description:
Launch a series of workloads and take memory metric measurements after
each launch.
Launch a series of workloads and take pod query-response latency measurements

Choose a reason for hiding this comment

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

I think query-response is fine (although you may wish to add 'http' or whatever protocol is being used to it). Without saying query-response, then I don't think we'd know which latency was being measured (network, boot, etc.).

Launch a series of workloads and take memory metric measurements after
each launch.
Launch a series of workloads and take memory metric measurements using 'collectd'
after each launch.

Choose a reason for hiding this comment

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

In fact, we don't just take memory measurements any more - I think saying a set of system level measurements using collected will now be more accurate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants