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

ai/live: Reuse HTTP connections for trickle requests #3470

Merged
merged 3 commits into from
Mar 26, 2025

Conversation

j0sh
Copy link
Collaborator

@j0sh j0sh commented Mar 25, 2025

Initially we were re-creating connections to avoid being stuck in a degraded state over time, but it seems that we may have trouble even establishing a connection sometimes. To mitigate this problem, try to re-use established connections.

Both scenarios should be caught by the slow orchestrator checker, but hopefully connection reuse makes the checker trip less often.

Initially we were re-creating connections to avoid being stuck
in a degraded state over time, but it seems that we may have
trouble even establishing a connection sometimes. Attempt to
re-use established connections to mitigate this problem.

Both scenarios should be caught by the slow orchestrator checker,
but hopefully connection reuse makes the checker trip less often.
@github-actions github-actions bot added the go Pull requests that update Go code label Mar 25, 2025
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Project coverage is 30.36115%. Comparing base (a4b76be) to head (c335356).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
trickle/trickle_publisher.go 0.00000% 13 Missing ⚠️
trickle/trickle_subscriber.go 0.00000% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3470         +/-   ##
===================================================
+ Coverage   30.35863%   30.36115%   +0.00252%     
===================================================
  Files            151         151                 
  Lines          44531       44524          -7     
===================================================
- Hits           13519       13518          -1     
+ Misses         30222       30216          -6     
  Partials         790         790                 
Files with missing lines Coverage Δ
trickle/trickle_subscriber.go 0.00000% <0.00000%> (ø)
trickle/trickle_publisher.go 0.00000% <0.00000%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4b76be...c335356. Read the comment docs.

Files with missing lines Coverage Δ
trickle/trickle_subscriber.go 0.00000% <0.00000%> (ø)
trickle/trickle_publisher.go 0.00000% <0.00000%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

// ignore orch certs for now
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}}).Do(req)
resp, err := httpClient().Do(req)
Copy link
Contributor

Choose a reason for hiding this comment

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

just checking it was intentional to create a new client on this one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep - this method is not on the hot path and actually only gets invoked if there is a problem sending the segment. So it seems best to have a fresh client here to mitigate any issues with the existing connection, similar to why we reset the client when we retry (here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some comments clarifying this, thanks - c3330b0

@j0sh j0sh enabled auto-merge (squash) March 26, 2025 16:39
@j0sh j0sh merged commit 6d8b5c3 into master Mar 26, 2025
16 checks passed
@j0sh j0sh deleted the ja/trickle-httpclient branch March 26, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants