Skip to content

Update load generation logic#1582

Merged
latobarita merged 8 commits intostellar:masterfrom
marta-lokhova:load_testing
Mar 23, 2018
Merged

Update load generation logic#1582
latobarita merged 8 commits intostellar:masterfrom
marta-lokhova:load_testing

Conversation

@marta-lokhova
Copy link
Copy Markdown
Contributor

-- Description
* Refactor LoadGenerator to achieve the following:
- Create accounts in a separate step
- Be able to batch up to 100 ops/tx
- Create single payments in a separate step
- Deterministically generate destination account based on the source account and its sequence number
- Mark load generation complete after changes are propagated to the DB
- Update tests to use the new load generator

-- Usage
* A user would hit generateload endpoint with the following params:
- mode [create|pay] - determines accounts creation or payments
- nAccounts - number of accounts to create or use for payments
- nTxs - num payments to perform
- batchsize - when creating accounts, batch multiple ops in a transaction
- txrate - tx/s injestion rate, (num or "auto")
* Note that accounts creations and payments are separate calls to the endpoint

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that current version of updateSequenceNumber on master breaks the code (because of usage of REQUIRE), so this change is copied from bump sequence PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that necessary here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It ensures mRoot is set, in case an outside method calls clear before generateLoad.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that name and return value of this method are misleading.

This method does not return root account, and its return value is never used.
I propose to either:

  • rename it to void LoadGenerator::createRootAccount
  • make it return res or nullptr, remove mRoot field and pass returned value to creationTransaction as a parameter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In what case is that true?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Never mind, found that case:)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks for me as if there are 2 different methods hiding inside this loop, one for isCreate == true and one for == false. Or maybe it is even small interface with 2 implementation. Following all those if (isCreate) branches one after another is quite hard.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it deserves separate method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could just make std::vector return type and result result.
RVO will take care of copying and having that gives us enough information to get the synced value as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is duplicated in CommandHandler, I think we need to extract that to somewhere

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about displaying message that such values of batchSize are illegal?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like name of this function is misleading - mode is not Num

@marta-lokhova
Copy link
Copy Markdown
Contributor Author

Thanks for reviewing! Will update the code based on the suggestions.

@vogel
Copy link
Copy Markdown
Contributor

vogel commented Mar 9, 2018

Looks good to me!
Could you rebase it on current master and squash some commits?

@marta-lokhova marta-lokhova force-pushed the load_testing branch 2 times, most recently from 341d372 to 4888e8e Compare March 13, 2018 02:34
@MonsieurNicolas
Copy link
Copy Markdown
Contributor

Marking this PR as dependent on #1533 (for the changes in test accounts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo - function arguments should start with lower case

@MonsieurNicolas
Copy link
Copy Markdown
Contributor

r+ 662d653

@marta-lokhova
Copy link
Copy Markdown
Contributor Author

@MonsieurNicolas looks like a clang-format issue. Fixed now.

@MonsieurNicolas
Copy link
Copy Markdown
Contributor

r+ 31a038d

@MonsieurNicolas
Copy link
Copy Markdown
Contributor

@latobarita: retry

latobarita added a commit that referenced this pull request Mar 23, 2018
Update load generation logic

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit 31a038d into stellar:master Mar 23, 2018
@marta-lokhova marta-lokhova deleted the load_testing branch August 28, 2018 22:42
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.

4 participants