-
Notifications
You must be signed in to change notification settings - Fork 315
Improve HTTP transport stability & logging #956
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
Hi @VinayakTiwari1103 - workflow approved. Could you please comment in the PR description how you tested these changes? We currently do not have long-running tests that can be launched in the CI workflow, we usually run another set of tests separately on real-world services for larger changes like this one. In reply to: 2708453414 |
I've updated the PR description with details on how I tested these changes. Let me know if anything else is needed. |
FYI, the end to end test is failing due to a known issue, fix will be merged today. The engine tests do not have any known issues. |
For the PR description update - thank you, that is helpful. In addition, I'd like to better understand the specific bugs you have fixed with this change. This will help me suggest new regression tests. Also, please split the cosmetic improvements into a separate PR (for example, changes to output formatting, spacing only changes, non-functional code rewrites, etc.). This will make it easier to review these and make sure they are consistent with the conventions of the project. Thanks, Marina |
Hi @marina-p, Thanks for the feedback and Guidance! Here’s a more detailed breakdown of the specific issues that this PR addresses: 1. Fixed socket reconnection issues after timeouts
2. Addressed connection closure inconsistencies across different OS environments
3. Improved request throttling logic to prevent API overload and optimize retries
I’ve also noted your request to separate cosmetic changes into a different PR and will do so accordingly. Regarding the failing end-to-end tests, I’ll re-run them once the known issue fix is merged and update the PR accordingly. Let me know if you need any further clarifications! Best, |
Hi @marina-p, I wanted to check if there are any further updates needed from my side for this PR. I have already addressed your feedback regarding the specific fixes and separated the cosmetic changes into a different PR. Also, I noticed that the required checks are still pending. Could you please confirm if there are any additional steps I should take, or if this is something that needs to be addressed from the maintainers' side? Let me know if any other modifications or clarifications are required! Best, |
I see two commits in the PR, but they appear identical for me. Could you please check that you pushed the commit with the latest updates? Let me know if I'm missing something. RE: the checks, apologies for the delay. I am still trying to figure out how to enable checks to trigger automatically for anyone who submits a PR (this was the case when the PR checks were linked to an ADO project, but no longer after migrating to GitHub actions). For now, the fastest option will be for you to run unit tests locally and get them to pass. Thanks, Marina |
Hi @marina-p, Thanks for reviewing the PR! I'll double-check the commits and ensure that the latest updates are correctly pushed. I'll update you shortly once I've verified. Regarding the checks, no worries! I'll run the unit tests locally and confirm that everything passes. Let me know if there’s anything else I should keep in mind while running them. Best, |
This PR enhances the HTTP transport layer by improving socket handling, request throttling, and error logging. It ensures better stability, optimizes reconnections, and improves debugging capabilities.
Key Changes
Testing Details