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

[Forge] Fix local mode forge run #2153

Merged
merged 2 commits into from
Jul 22, 2022
Merged

[Forge] Fix local mode forge run #2153

merged 2 commits into from
Jul 22, 2022

Conversation

sitalkedia
Copy link
Contributor

@sitalkedia sitalkedia commented Jul 22, 2022

Description

In local mode, submit lower TPS so that the transaction emitter is stable and also don't check for TPS/latency criteria.

Test Plan

FORGE_RUNNER_MODE=local ./testsuite/run_forge.sh

This change is Reviewable

@sitalkedia sitalkedia added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 22, 2022
@@ -168,15 +169,19 @@ if [ -n "$AVG_TPS" ]; then
echo "forge_job_avg_tps {FORGE_CLUSTER_NAME=\"$FORGE_CLUSTER_NAME\",FORGE_NAMESPACE=\"$FORGE_NAMESPACE\",GITHUB_RUN_ID=\"$GITHUB_RUN_ID\"} $AVG_TPS" | curl -u "$PUSH_GATEWAY_USER:$PUSH_GATEWAY_PASSWORD" --data-binary @- ${PUSH_GATEWAY}/metrics/job/forge
if [[ "$AVG_TPS" -lt "$TPS_THRESHOLD" ]]; then
echo "(\!) AVG_TPS: ${avg_tps} < ${TPS_THRESHOLD} tps"
FORGE_EXIT_CODE=1
if [ "$FORGE_RUNNER_MODE" != "local" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this perf threshold checking into the performance test itself ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that, but that will require some plumbing in the transaction emitter. May be as a follow up I can take a look at this?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm ok yea. this script is just getting too complicated

testsuite/run_forge.sh Outdated Show resolved Hide resolved
@sitalkedia sitalkedia force-pushed the forge_cli_local_mode branch from e8d476d to 39bc7bd Compare July 22, 2022 04:20
@sitalkedia sitalkedia force-pushed the forge_cli_local_mode branch from 39bc7bd to 2d136d3 Compare July 22, 2022 04:23
@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@sitalkedia sitalkedia enabled auto-merge (squash) July 22, 2022 06:34
@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@rustielin rustielin removed the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 22, 2022
@rustielin rustielin disabled auto-merge July 22, 2022 17:53
@rustielin rustielin enabled auto-merge (squash) July 22, 2022 17:54
@github-actions
Copy link
Contributor

❌ Forge test failure

Forge is land-blocking

Forge test runner terminated

@github-actions
Copy link
Contributor

✅ Forge test success

Forge is land-blocking

all up : 5492 TPS, 3115 ms latency, 6200 ms p99 latency,no expired txns

@rustielin rustielin merged commit 77ad147 into main Jul 22, 2022
@rustielin rustielin deleted the forge_cli_local_mode branch July 22, 2022 18:11
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.

2 participants