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

define common TxStatus Class #122

Merged
merged 5 commits into from
Jul 16, 2018
Merged

Conversation

haojun
Copy link
Contributor

@haojun haojun commented Jul 10, 2018

This PR is related to #119 and #110

  1. Define a common TxStatus class and modify all adapters accordingly.
  2. Reduce memory usage of local-client.js

Signed-off-by: Haojun Zhou [email protected]

@haojun
Copy link
Contributor Author

haojun commented Jul 10, 2018

Hi @nklincoln @stinger112 @Ram-srini , please help to review this PR to check if your adapters (composer, iroha, sawtooth) have been modified correctly and can run with no error.

Thanks 😀

@haojun
Copy link
Contributor Author

haojun commented Jul 10, 2018

And I'll try to modify the result processing base on this PR to reduce client memory consumption for better long time test supporting.

@nklincoln
Copy link
Contributor

nklincoln commented Jul 11, 2018

Hi @haojun,
Whilst testing this I noticed that the change made in local-client.js has broken the ability to run composer tests.

The current while loop within runFixedNumber isn't behaving quite as expected. I made a change to a for loop, conditioned on the required number of invokes. I think the unexpected operation is because the global variable txNum relies upon an update performed once the cb.run() has completed, which means it is possible to send in excess of the number of intended invokes if the cb.run() duration is in excess of the apply rate control duration ... as is current using a while loop.

The required change for test completion (inclusive of changes in this PR) is:

for(let i = 0 ; i < msg.numb ; i++) {
        promises.push(cb.run().then((result) => {
            addResult(result);
            return Promise.resolve();
        }));
        await rateControl.applyRateControl(start, txNum, results);
    }

@aklenik
Copy link
Contributor

aklenik commented Jul 11, 2018

Although the Fabric query API is hardcoded right now, in the src/fabric/e2eUtils.js file before L771 a txStatus.SetVerification(true), and before L781 a txStatus.SetVerification(false) is missing (although the latter is the default behavior). But it's not critical since this part needs to be implemented properly anyway.

@haojun
Copy link
Contributor Author

haojun commented Jul 12, 2018

@nklincoln,

By #66 a 'submitCallback' is injected into the output of getcontext() to count the actual tx submitting(see from https://github.com/hyperledger/caliper/blob/6131f4fca8881d9bb2f3a0fdc829b7df7036659e/src/comm/client/local-client.js#L165).

Sorry I forgot to change composer accordingly, and I think that's the real cause of the composer test failure.

Can you check and help to fix it? Thanks.

@nklincoln
Copy link
Contributor

@haojun, ok - thanks... in which case the 'quick fix' there is to modify composer.js submitTransaction() method to include:

submitTransaction(connection, transaction) {
        let invoke_status = new TxStatus(transaction.getIdentifier());
        if(connection.engine) {
            connection.engine.submitCallback(1);
        }
       return connection.submitTransaction(transaction)
...

This is likely to have emerged due to the bad naming convention used in the composer plug, which does not retain the invokeSmartContract convention used by the other plugins ... happy to raise a PR and fix this if desired.

@haojun
Copy link
Contributor Author

haojun commented Jul 13, 2018

@nklincoln done

Haojun Zhou added 3 commits July 13, 2018 13:47
Signed-off-by: Haojun Zhou <[email protected]>
Signed-off-by: Haojun Zhou <[email protected]>
Signed-off-by: Haojun Zhou <[email protected]>
@haojun haojun requested review from a team and panyu4 and removed request for a team July 13, 2018 05:56
Haojun Zhou added 2 commits July 16, 2018 10:01
Signed-off-by: Haojun Zhou <[email protected]>
Signed-off-by: Haojun Zhou <[email protected]>
@haojun
Copy link
Contributor Author

haojun commented Jul 16, 2018

Checked with Sawtooth and Iroha

@panyu4 panyu4 merged commit 6cf2891 into hyperledger-caliper:master Jul 16, 2018
@nklincoln nklincoln mentioned this pull request Jul 16, 2018
@JulienGuo
Copy link

JulienGuo commented Jul 16, 2018

thanks @nklincoln
Can you help me to add a new feature of pidrate, I want change the invoke timeout from a constant value to a variable value , and Please make the value related to sleep time.
#131

faustovanin pushed a commit to faustovanin/caliper that referenced this pull request Oct 27, 2023
…ad-modules

Update benchmark workload modules to v0.4.0
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.

5 participants