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

fix: Disable connection pool to fix dist server feature #1612

Merged
merged 12 commits into from Feb 22, 2023

Conversation

Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented Feb 19, 2023

The root cause of dist server restart failure seems related to request's http keep-alive logic.

More details could be found here: #1563

In the future, we should use the same global client instead and bring connection pool back.

Signed-off-by: Xuanwo <github@xuanwo.io>
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Base: 30.80% // Head: 30.77% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (a426f30) compared to base (b0dd958).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head a426f30 differs from pull request most recent head 7905216. Consider uploading reports for the commit 7905216 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1612      +/-   ##
==========================================
- Coverage   30.80%   30.77%   -0.04%     
==========================================
  Files          49       49              
  Lines       16661    16667       +6     
  Branches     8061     8063       +2     
==========================================
- Hits         5133     5129       -4     
+ Misses       6242     6237       -5     
- Partials     5286     5301      +15     
Impacted Files Coverage Δ
src/compiler/compiler.rs 36.10% <ø> (-0.08%) ⬇️
src/dist/client_auth.rs 0.00% <0.00%> (ø)
src/dist/http.rs 0.00% <0.00%> (ø)
src/util.rs 38.43% <0.00%> (+0.29%) ⬆️
tests/dist.rs 100.00% <ø> (ø)
tests/harness/mod.rs 44.18% <ø> (ø)
src/test/utils.rs 36.36% <0.00%> (-4.05%) ⬇️
src/compiler/c.rs 37.93% <0.00%> (-0.46%) ⬇️
src/compiler/gcc.rs 54.13% <0.00%> (-0.31%) ⬇️
src/lru_disk_cache/mod.rs 41.15% <0.00%> (-0.31%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title WIP: Debug for dist server feature fix: Disable connection pool to fix dist server feature Feb 19, 2023
@Xuanwo Xuanwo marked this pull request as ready for review February 19, 2023 14:52
@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Feb 19, 2023

I have repeated the test of dist server several times, it is now stable!

@sylvestre
Copy link
Collaborator

excellent, thanks. let's see if it sticks.

@sylvestre
Copy link
Collaborator

what is the impact for the users ?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Feb 20, 2023

what is the impact for the users ?

No difference. We already send Connection: close for each response, so the current behavior should have no changes.

src/util.rs Show resolved Hide resolved
Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

One nit, other than that, looks good to me

Signed-off-by: Xuanwo <github@xuanwo.io>
@sylvestre
Copy link
Collaborator

A job is failing to build. Maybe related to the fs err change?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Feb 22, 2023

A job is failing to build. Maybe related to the fs err change?

Fixed now.

Signed-off-by: Xuanwo <github@xuanwo.io>
src/compiler/compiler.rs Outdated Show resolved Hide resolved
sylvestre and others added 2 commits February 22, 2023 14:15
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
@drahnr drahnr merged commit 520a30b into mozilla:main Feb 22, 2023
@Xuanwo Xuanwo deleted the restart-dist-debug branch February 22, 2023 16:08
@sylvestre
Copy link
Collaborator

Thanks for the squash :)

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.

None yet

4 participants