Closed maddisondavid closed 4 years ago
@maddisondavid I have clones your branch and tried this command:
bin\pravega-benchmark -controller tcp://localhost:9090 -consumers 1 -producers 1 -events 100 -size 10 -stream test1 -time 60 -readthroughputcsv read.csv -writethroughputcsv write.csv
After the execution of this command, I do see the read.csv
file but I do not see the write.csv
. Is this because we only can create CSV for either write or read, or maybe this PR has introduced some problem to the -writethroughputcsv
option?
The same happens if I use in addition -readcsv
and -writecsv
. I only see the "read" latency file being created, and none of the other expected files (read and write throughput, write latency):
bin\pravega-benchmark -controller tcp://localhost:9090 -consumers 1 -producers 1 -events 100 -size 10 -stream test1 -time 60 -writethroughputcsv write.csv -readcsv read -writecsv write
Cannot we have more than one CSV reporter at a time?
I wasn't aware of any changes to the existing readcsv
and writecsv
options introduced as part of this issue, however I will check the code again and validate. As you point out, it should be possible to use both at the same time.
This seems to have been a change introduced in commit 169fa2bc68d62ccd7484ee053e0a4e864c3a59d3
back in April 2019
https://github.com/pravega/pravega-benchmark/blob/master/src/main/java/io/pravega/perf/PravegaPerfTest.java#L301-L307
writeAndRead = consumerCount > 0;
if (writeAndRead) {
produceStats = null;
} else {
produceStats = new PerfStats("Writing", reportingInterval, messageSize, writeFile, writeThroughputFile);
}
This means that the producerStats will only be created IF we have producers and NO consumers. It seems to be a deliberate change because it's also repeated when the PerfStats are started, however I don't know why it was changed this way. https://github.com/pravega/pravega-benchmark/blob/master/src/main/java/io/pravega/perf/PravegaPerfTest.java#L339-L346
public void start(long startTime) throws IOException {
if (produceStats != null && !writeAndRead) {
produceStats.start(startTime);
}
@RaulGracia I've fixed the conflicts, the change to only write the CSV files for reads or writes was made by a previous commit (not this branch). If we want to revert it then I think that's best handled by another PR.
In terms of the constants, if you want me to change it I will, but I don't think it would add anything (they're only used as labels).
Change log description Adds an extra performance recorder to record Throughput while the test is running
Purpose of the change Fixes #87
What the code does Adds a Throughput writer to the Performance recorder that records the number of bytes every second. This is crucial for performing Historical Reader performance tests. At the end of the test the throughput percentiles are reported in a human readable format.
Also adds two new options for specifying a CSV file of the throughput for each second the test is running:
Recording Throughput The CSVThroughput recorder records the Throughput (in MiB/) using the current
TimeWindow
. If noreportingIntervalMillis
is specified, the default value of 5 seconds is used.How to verify If the
readthroughputcsv
orwritethroughputcsv
options are specified then the bytes per reporting period are written to a CSV file for every second the test is running:Signed-off-by: David Maddison david.maddison@dell.com