Implementation of warm-ups + sync'd start times

Implementation of warm-ups + sync'd start times

[kirill.sc, 2017-08-19 19:16:20]

Hey dude,

I just noticed your review on the website. I haven't received the email
notification. What I usually do to avoid situations when email was not send for
some reason for new reviews, is to click on "Publish + Mail Draft Comments"
check that both myself and the reviewers are in the list and then click send.
And check that I got gmail along with the reviewer CC's.

Will start looking into it now. 
Best,
Kirill

[kirill.sc, 2017-08-20 10:00:01]

see first set of comments

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
File core/src/main/java/com/yahoo/ycsb/Client.java (right):

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/Client.java:389: private static long
sync_start_timestamp;
always try to add resolution to variables dealing with time, i.e., ms, ns, sec,
etc.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/Client.java:467:
System.out.println("Initiating synchronization sleep for " +
(sync_start_timestamp - initTime)*1000000 + " ms");
You can print the timestamp as a real date to know how long you sleep until, and
then print the remaining time in seconds instead of ms, the latter will make it
easier to read for a human.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/Client.java:468: sleepUntil(System.nanoTime()
+ (sync_start_timestamp - initTime)*1000000);
it is often difficult to see how many zeros do you have, and easy to overlook an
error. 
you can do something like 1000*1000*1000 or use 1e9 notation.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
File core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java (right):

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java:802:
if(!sendasync) {
Here and below, it is a common not to start "if" statements with negations.
Easier to read. You can swap the order of conditions.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java:819:
if(!sendasync) {
Here and abobe, why do we have these conditions here? These seems to be related
to sync and async but not related to wamp up time or synchronization? Why do we
need to differenctiate them here?

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
File core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java (right):

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:44: private
static final String WARMUP_PERIOD_PROPERTY="warmupperiod";
I would add "sec" in the name of the name of the property, e.g., "warmuptimesec"
or "warmupperiods_sec" warmupseconds ....

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:49: private
static final String WARMUP_PERIOD_PROPERTY_DEFAULT="0";
If you add SEC in the name of the variable, then comment become redundant.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:54: public static
final String THREAD_COUNT_PROPERTY = "threadcount";
This property name is confusing. Are you using a different number of threads for
warm up? how it is different from the "threads"? Add readme for it as well.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:89: if(timestamp
== null)
here and in other places I would avoid if statements without scope brackets {}.
Very easy to make a mistake.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:92: if(warmuprate
== -1 && timestamp>=60000000000L)
use scientific notation

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:93: warmuprate =
lineCount/60;
dont you have to break out of this while true loop here? Otherwise you will go
until the end of the trace. You only want to go up to 1 minute.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:103: " Experiment
will terminate once end of trace is reached.");
Is it safe to ignore this warning if we use traceworkload?  If it is then
perhaps we should state it.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:112:
System.err.println("Error: Failure to start warmup phase. Trace duration is less
than 1 minute.");
The if statement above does not corresponds to the error message. How
(warmuprate == -1 && warmupperiod_ns>0) means that duration is less than 1
minute?
- We should probably check this case right after the while(true) loop

[kirill.sc, 2017-08-20 13:06:31]

see my last set of comments

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
File core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java (right):

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:121: long
stepinterval = warmupperiod_ns/5; //For now we statically set the step function
to have 5 steps
make 5 a global variable in this class. I would think it would make sense to
have a more gradual increase. say 20 steps or something like that. to start very
slow.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:124: int stepx =
(int) (stepincrement * stepinterval/1000000000L);
do stepinterval/1000000000L once when you initialize the variable

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:130:
stepFunc.put(stepx, stepy);
what are the stepx and stepy?

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:154: public long
getDeadlineForTransaction(long startTimeNs, int opsdone, long targetOpsTickNs) {
I dont understand the input variables, what do they mean? 
Is this function being called for every request?

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:155: if(opsdone
>= warmupops) {
Do we guaranteed to be here synchronously between different YCSBs? Is there
anything that can prevent one of the YCSBs to come here at the same time. For
example, if one of the YCSBs using sync mode and has not sufficient threads then
it will take more time to come to the point when all warming up operations are
completed.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:156: //we are
done warming up
When we are here for the first time, it would be nice to see a message in YCSB
saying, warming up is over, now starts the real deal.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:172: {
be consistent with your scope brackets are they on the same line or on the next
line? I personally prefer to have them on the same line. Saves space.

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:173: if(opsdone >
stepopcount) {
these two if statements are very difficult to read. 
Why do we have to iterate over stepFunc? can't we just know where we are based
on the time since the start or the number of operations we have sent?

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:202: double
targetperthread = ((double) target) / ((double) threadcount);
targetperthread -> targetSendingRatePerThreadPerSecond  ??

https://codereview.appspot.com/324260043/diff/1/core/src/main/java/com/yahoo/...
core/src/main/java/com/yahoo/ycsb/workloads/TraceWorkload.java:206: return
targetOpsTickNs;
what is this value targetOpsTickNs ? is this how much each thread will wait
until sending a new request?