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

W-17333982: fix for HTTP:TIMEOUT error #961

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

neetya26
Copy link

@neetya26 neetya26 commented Nov 29, 2024

Screenshot 2024-12-02 at 6 26 51 PM Screenshot 2024-12-03 at 12 01 24 PM

Attaching before and after images for the fix

@neetya26 neetya26 requested a review from a team as a code owner November 29, 2024 14:46
@@ -262,7 +262,7 @@ private void doRequestWithRetry(HttpExtensionClient client, HttpRequesterConfig
}

logger.error(getErrorMessage(httpRequest));
HttpError error = exception instanceof TimeoutException ? TIMEOUT : CONNECTIVITY;
HttpError error = exception instanceof TimeoutException ? REQUEST_TIMEOUT : CONNECTIVITY;

Choose a reason for hiding this comment

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

As discussed, Currently we are throwing HTTP:Timeout in case of no response, after this change we will throw HTTP:REQUEST_TIMEOUT.
As per earlier implementation customers might be expecting HTTP:Timeout here so this might break few things on their side. It would suggest to keep it same.
For the scenario where status code is 408, we can try if HTTP:Timeout can be thrown there as well to make sure it is same everywhere.
If above is not possible then can go ahead with REQUEST_TIMEOUT at both places and need to update the documentation as well.

Copy link
Contributor

@eze210 eze210 left a comment

Choose a reason for hiding this comment

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

Please add the corresponding MUnit test case

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.

3 participants