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

[Bench] Fix a few issues in benchmark #952

Merged
merged 1 commit into from
Mar 20, 2022
Merged

[Bench] Fix a few issues in benchmark #952

merged 1 commit into from
Mar 20, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Mar 19, 2022

This PR fixes two issues:

  1. When we are benchmarking sending both transaction and cert, each tx and their cert pair must be sent in order.
    When we use multiple TCP connections, requests are broken into chunks and each chunk will be sent in parallel. We need to make sure that when we break them into chunks, we don't break any tx/cert pair, which would end up cert sending earlier before tx, causing tx failures. Added an assertion for this to make sure correct config is provided.
  2. The take_while condition in batch_send_one_chunk is incorrect. It would always miss the last response. Fix it.

@lxfind lxfind force-pushed the fix-bench-invariant branch from 089dd5f to de1b281 Compare March 19, 2022 01:33
@lxfind lxfind changed the title [Bench] Check parameters to make sure tx and cert are sent in order [Bench] Fix a few issues in benchmark Mar 19, 2022
@lxfind lxfind force-pushed the fix-bench-invariant branch from de1b281 to ea8278b Compare March 19, 2022 04:09
@lxfind lxfind force-pushed the fix-bench-invariant branch from ea8278b to 3814581 Compare March 19, 2022 19:05
Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Good fixes!

@lxfind lxfind merged commit 29edde2 into main Mar 20, 2022
@lxfind lxfind deleted the fix-bench-invariant branch March 20, 2022 16:07
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