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.

Sign up for GitHub

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

Curl keep alive #5930

Closed
wants to merge 29 commits into from
Closed

Curl keep alive #5930

wants to merge 29 commits into from

Conversation

gearama
Copy link
Member

@gearama gearama commented Aug 20, 2024 •
edited
Loading

closes #5877

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@gearama gearama marked this pull request as ready for review August 26, 2024 21:57
@gearama gearama requested a review from LarryOsterman August 29, 2024 22:06
@RickWinter
Copy link
Member

Does this fix address issue 5877

@gearama gearama requested a review from RickWinter August 30, 2024 20:51
@LarryOsterman
Copy link
Member

I have a few comments, but my biggest concern is that we don't have any tests which validate this code against an actual server which returns keep-alive responses, thus there may be assumptions being made in this implementation which are not supported in actual practice.

Do we know if storage or other servers we use returns keep-alive headers which we could use to help test?

@gearama
Copy link
Member Author

gearama commented Sep 4, 2024 •
edited
Loading

I have a few comments, but my biggest concern is that we don't have any tests which validate this code against an actual server which returns keep-alive responses, thus there may be assumptions being made in this implementation which are not supported in actual practice.

Do we know if storage or other servers we use returns keep-alive headers which we could use to help test?

since i agree with this statement, i don't think we should merge this change, even if on surface it might look ok, i'd rather not merge it since we cannot get real life validation of the approach. as such i'll leave it there if someone else wants to take a stab. Also since my vacation is coming up i don't want to merge something of this impact and then vanish for a few weeks.
Please leave a comment if you agree.

@ckairen
Copy link
Member

ckairen commented Oct 7, 2024

/azp run cpp - core - ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core-cpp

@gearama gearama closed this Nov 25, 2024
@gearama gearama deleted the CurlKeepAlive branch March 31, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RickWinter RickWinter RickWinter left review comments

@LarryOsterman LarryOsterman LarryOsterman left review comments

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan

@antkmsft antkmsft Awaiting requested review from antkmsft antkmsft is a code owner

Assignees
No one assigned
Labels
Azure.Core
Projects
None yet
Milestone
No milestone
5 participants
@gearama @RickWinter @LarryOsterman @ckairen @azure-sdk