-
Notifications
You must be signed in to change notification settings - Fork 334
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: Added connection error timeout #485
Conversation
Signed-off-by: Fraser Benjamin <[email protected]>
CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA |
I have read the CLA Document and I hereby sign the CLA |
We have a function called That will use the authentication error timeout, would it not be better to extend that and then maybe change the error message to mention authentication/connection? |
Signed-off-by: Fraser Benjamin <[email protected]>
Signed-off-by: Fraser Benjamin <[email protected]>
Hey, great suggestion thank you, I've just updated the PR to renamed the timeout and handle the additional errors that come from SQS. If there's anything else you'd like me to change please let me know! |
Code Climate has analyzed commit b9554a7 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 90.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 98.2% (0.0% change). View more on Code Climate. |
Sorry, I have been delayed somewhat in looking at this, I'm not on this full time. Also, looks like I might have confused a little bit, the changes here are a bit too much for what's actually needed here, we just need to extend the conditional, we also don't want Connection Error to be true for every error, using SQSError would mean that every error would be treated as a connection error. I've opened a PR here with the change that I think is needed, I'll close this one in favour of that: #489 |
Resolves #484
Description:
Adds a timeout when a connection to SQS fails due to a lack of internet connection, invalid queue name etc.
Type of change:
Why is this change required?:
Currently when a connection fails it will be immediately retried causing an instant loop of retries with lots of unnecessary requests and error events.
Code changes:
This pull request primarily involves renaming "authentication errors" in the codebase, changing them to "connection errors". and adding the additional failure error events. This includes changes in variable names, method parameters, and error handling logic. The changes are spread across the
src/consumer.ts
,src/types.ts
,src/errors.ts
, andtest/tests/consumer.test.ts
files.Here are the most important changes:
Renaming and refactoring:
src/consumer.ts
: Renamed theauthenticationErrorTimeout
variable toconnectionErrorTimeout
in theConsumer
class and updated the corresponding assignment and usage in error handling. [1] [2] [3]src/types.ts
: Updated theConsumerOptions
interface to renameauthenticationErrorTimeout
toconnectionErrorTimeout
.src/errors.ts
: Updated theisConnectionError
function to include more error codes that are considered as connection errors.test/tests/consumer.test.ts
: Updated the test cases to useCONNECTION_ERROR_TIMEOUT
instead ofAUTHENTICATION_ERROR_TIMEOUT
. Also, added a test case to handle connection errors. [1] [2] [3]These changes appear to broaden the scope of errors considered as connection issues and handle them uniformly across the application.
Checklist: