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

Adding the keying/think time to each transaction. #7

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

psudheer21
Copy link
Contributor

Summary:
Added support for keying and think times for each transaction that can
be controlled using a configuration parameter.

Changed the default configuration values to support running the workload
optimally according to the specs.

Changed some part of the Load workload to strictly follow the standard.

Reviewers:
Neha, Karthik

Summary:
Added support for keying and think times for each transaction that can
be controlled using a configuration parameter.

Changed the default configuration values to support running the workload
optimally according to the specs.

Changed some part of the Load workload to strictly follow the standard.

Reviewers:
Neha, Karthik
Copy link
Contributor

@rkarthik007 rkarthik007 left a comment

Choose a reason for hiding this comment

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

Nice work! First pass of comments.

@@ -196,6 +196,51 @@ synchronized public void setCurrStatement(Statement s) {
this.currStatement = s;
}

private long getKeyingTime(TransactionType type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to add a comment on top of this (and the next) function with the TPCC spec and the relevant section that specifies these values.

Also, perhaps consider renaming this function with the units at the end, such as: getKeyingTimeMillis() if it is in millis.

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.

// Wait for the keying time which is a fixed number for every type of transaction.
long keying_time_msecs = getKeyingTime(transactionTypes.getType(pieceOfWork.getType()));
try {
long start_sleep = System.nanoTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this variable and the following LOG.info to measure the sleep time? We should get rid of these I feel.

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 did this for debugging.
WOuldn't be part of the final code.

// Sleep for the think time duration.
long think_time_msecs = getThinkTime(transactionTypes.getType(pieceOfWork.getType()));
try {
long start_sleep = System.nanoTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debugging. Won't be in the final code.

@ndeodhar ndeodhar changed the title [WIP] Adding the keying/think time to each transaction. Adding the keying/think time to each transaction. Apr 6, 2020
Copy link
Contributor

@ndeodhar ndeodhar left a comment

Choose a reason for hiding this comment

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

LGTM

private double scaleFactor = 1.0;
private double selectivity = -1.0;
private int terminals;
private int loaderThreads = ThreadUtil.availableProcessors();
private int numTxnTypes;
private TraceReader traceReader = null;
private boolean useKeyingTime = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

With these defaults, we'll use keying and think time for other workloads like workload_1, workload_2 etc. also. Just want to make sure that it's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

workload_all should be what we should run now. And all of our defaults should be to make it run easy for that.
For the other workloads we can still make it false and then run it.

warehouse.w_state = TPCCUtil.randomStr(3).toUpperCase();
warehouse.w_zip = "123456789";
warehouse.w_state = TPCCUtil.randomStr(2).toUpperCase();
warehouse.w_zip = TPCCUtil.randomNStr(4) + "11111";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this to the description as well - since your diff includes more than just the changes for think/keying time.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment already had these details/. {point 3 in the message above}

@psudheer21 psudheer21 merged commit 658a8a2 into master Apr 7, 2020
@psudheer21 psudheer21 deleted the AddKeyingAndThinkTimes branch April 7, 2020 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants