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

Jump to bottom

benchmark: fix http2 benchmarks #36871

Merged
merged 0 commits into from Jan 11, 2021
Merged

benchmark: fix http2 benchmarks #36871

merged 0 commits into from Jan 11, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 10, 2021

Fixes: #36746

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http2 Issues or PRs related to the http2 subsystem. labels Jan 10, 2021
@Trott Trott mentioned this pull request Jan 10, 2021
Closed
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 10, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35424/

@Trott
Copy link
Member Author

Trott commented Jan 10, 2021 •
edited

Welp...I tried running the http2 benchmarks with these changes and I still got EADDRNOTAVAIL...so the new theory is...uh...working on it....

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 10, 2021
@Trott
Copy link
Member Author

Trott commented Jan 10, 2021

Looks like the http2 benchmarker emits errors on connection but the http and https benchmarkers swallow them. I've added a commit to swallow connection errors on http2, and will run the benchmarks now to see if that causes any surprising results.

@Trott
Copy link
Member Author

Trott commented Jan 10, 2021

Looks like the http2 benchmarker emits errors on connection but the http and https benchmarkers swallow them. I've added a commit to swallow connection errors on http2, and will run the benchmarks now to see if that causes any surprising results.

That seems to be working.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35430/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2021 •
edited by Trott

CI: https://ci.nodejs.org/job/node-test-pull-request/35431/ ✔️

@Trott Trott mentioned this pull request Jan 11, 2021
Closed
2 tasks
@Trott
Copy link
Member Author

Trott commented Jan 11, 2021

I'd like to fast-track this so I can follow up on #36679 (comment). Please 👍 here to fast-track.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 11, 2021
@Trott Trott closed this Jan 11, 2021
@Trott
Copy link
Member Author

Trott commented Jan 11, 2021

Landed in 1c4fa9a

@Trott Trott deleted the fix-http2-benchmarks branch January 11, 2021 04:27
@Trott Trott merged commit 1c4fa9a into nodejs:master Jan 11, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@Trott @danielleadams
benchmark: fix http2 benchmarks
3ca7a78 
Fixes: #36746

PR-URL: #36871
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@Trott @targos
benchmark: fix http2 benchmarks
c22efc5 
Fixes: #36746

PR-URL: #36871
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
@Linkgoron Linkgoron mentioned this pull request May 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http2 benchmark seems broken
4 participants
@Trott @nodejs-github-bot @gireeshpunathil @aduh95