-
Notifications
You must be signed in to change notification settings - Fork 5
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
AP-3648 SQS exponential delay between retries #150
Changes from 28 commits
4d79570
7ba3667
e12151f
7846b7b
c2d9273
d0b4a47
08f2c04
b700281
2447665
5767360
ac03b1f
f0fffc3
38cc9a0
851c7c4
a63f334
edf12c0
3ba05da
bf51f7d
2b953eb
8c49c9d
d3a0fa8
c7b140e
cc06d95
9e79981
ad0d266
0e59805
e49731d
a5233c0
a6b619c
8cdbfe7
33dc101
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ import type { | |
TransactionObservabilityManager, | ||
} from '@message-queue-toolkit/core' | ||
import { | ||
isRetryDateExceeded, | ||
isMessageError, | ||
parseMessage, | ||
HandlerContainer, | ||
|
@@ -153,27 +152,26 @@ export abstract class AbstractAmqpConsumer< | |
.then((result) => { | ||
if (result.result === 'success') { | ||
this.channel.ack(message) | ||
this.handleMessageProcessed(originalMessage, 'consumed') | ||
this.handleMessageProcessed(parsedMessage, 'consumed') | ||
return | ||
} | ||
|
||
// retryLater | ||
const timestamp = this.tryToExtractTimestamp(originalMessage) ?? new Date() | ||
// requeue the message if maxRetryDuration is not exceeded, else ack it to avoid infinite loop | ||
if (!isRetryDateExceeded(timestamp, this.maxRetryDuration)) { | ||
if (this.shouldBeRetried(originalMessage, this.maxRetryDuration)) { | ||
// TODO: Add retry delay + republish message updating internal properties | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not implemented within this PR, adding todo |
||
this.channel.nack(message, false, true) | ||
this.handleMessageProcessed(originalMessage, 'retryLater') | ||
this.handleMessageProcessed(parsedMessage, 'retryLater') | ||
} else { | ||
// ToDo move message to DLQ once it is implemented | ||
this.channel.ack(message) | ||
this.handleMessageProcessed(originalMessage, 'error') | ||
this.handleMessageProcessed(parsedMessage, 'error') | ||
} | ||
}) | ||
.catch((err) => { | ||
// ToDo we need sanity check to stop trying at some point, perhaps some kind of Redis counter | ||
// If we fail due to unknown reason, let's retry | ||
this.channel.nack(message, false, true) | ||
this.handleMessageProcessed(originalMessage, 'retryLater') | ||
this.handleMessageProcessed(parsedMessage, 'retryLater') | ||
this.handleError(err) | ||
}) | ||
.finally(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import type { ZodSchema, ZodType } from 'zod' | |
import type { MessageInvalidFormatError, MessageValidationError } from '../errors/Errors' | ||
import type { Logger, MessageProcessingResult } from '../types/MessageQueueTypes' | ||
import type { DeletionConfig, QueueDependencies, QueueOptions } from '../types/queueOptionsTypes' | ||
import { isRetryDateExceeded } from '../utils/dateUtils' | ||
import { toDatePreprocessor } from '../utils/toDateProcessor' | ||
|
||
import type { | ||
|
@@ -42,11 +43,15 @@ export abstract class AbstractQueueService< | |
ExecutionContext = undefined, | ||
PrehandlerOutput = undefined, | ||
> { | ||
// Used to keep track of the number of retries performed on consumer | ||
private readonly messageNumberOfRetriesField = '_internalNumberOfRetries' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
BullMQ naming of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me! will apply the change thanks Igor 🙇 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmm, thinking about it, I am not really sure, this counting will only be applied on retryLater cases, meaning that errors will go with the SQS usual retry logic flow.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair point. maybe we can explain this with a jsdoc, without naming changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course! I will add it in a bit :D thanks Igor 🙏 |
||
// Used to avoid infinite retries on the same message | ||
protected readonly messageTimestampField: string | ||
|
||
protected readonly errorReporter: ErrorReporter | ||
public readonly logger: Logger | ||
protected readonly messageIdField: string | ||
protected readonly messageTypeField: string | ||
protected readonly messageTimestampField: string | ||
protected readonly logMessages: boolean | ||
protected readonly creationConfig?: QueueConfiguration | ||
protected readonly locatorConfig?: QueueLocatorType | ||
|
@@ -198,7 +203,45 @@ export abstract class AbstractQueueService< | |
return await barrier(message, executionContext, preHandlerOutput) | ||
} | ||
|
||
protected tryToExtractTimestamp(message: MessagePayloadSchemas): Date | undefined { | ||
shouldBeRetried(message: MessagePayloadSchemas, maxRetryDuration: number): boolean { | ||
const timestamp = this.tryToExtractTimestamp(message) ?? new Date() | ||
return !isRetryDateExceeded(timestamp, maxRetryDuration) | ||
} | ||
|
||
protected getMessageRetryDelayInSeconds(message: MessagePayloadSchemas): number { | ||
// if not defined, this is the first attempt | ||
const retries = this.tryToExtractNumberOfRetries(message) ?? 0 | ||
|
||
// exponential backoff -> (2 ^ (attempts)) * delay | ||
// delay = 1 second | ||
return Math.pow(2, retries) | ||
} | ||
|
||
protected updateInternalProperties(message: MessagePayloadSchemas): MessagePayloadSchemas { | ||
const messageCopy = { ...message } // clone the message to avoid mutation | ||
|
||
/** | ||
* If the message doesn't have a timestamp field -> add it | ||
* will be used to prevent infinite retries on the same message | ||
*/ | ||
if (!this.tryToExtractTimestamp(message)) { | ||
// @ts-ignore | ||
messageCopy[this.messageTimestampField] = new Date().toISOString() | ||
this.logger.warn(`${this.messageTimestampField} not defined, adding it automatically`) | ||
} | ||
|
||
/** | ||
* add/increment the number of retries performed to exponential message delay | ||
*/ | ||
const numberOfRetries = this.tryToExtractNumberOfRetries(message) | ||
// @ts-ignore | ||
messageCopy[this.messageNumberOfRetriesField] = | ||
numberOfRetries !== undefined ? numberOfRetries + 1 : 0 | ||
|
||
return messageCopy | ||
} | ||
|
||
private tryToExtractTimestamp(message: MessagePayloadSchemas): Date | undefined { | ||
Comment on lines
+212
to
+250
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Common logic here |
||
// @ts-ignore | ||
if (this.messageTimestampField in message) { | ||
// @ts-ignore | ||
|
@@ -213,6 +256,18 @@ export abstract class AbstractQueueService< | |
return undefined | ||
} | ||
|
||
private tryToExtractNumberOfRetries(message: MessagePayloadSchemas): number | undefined { | ||
if ( | ||
this.messageNumberOfRetriesField in message && | ||
typeof message[this.messageNumberOfRetriesField] === 'number' | ||
) { | ||
// @ts-ignore | ||
return message[this.messageNumberOfRetriesField] | ||
} | ||
|
||
return undefined | ||
} | ||
|
||
protected abstract resolveNextFunction( | ||
preHandlers: Prehandler<MessagePayloadSchemas, ExecutionContext, PrehandlerOutput>[], | ||
message: MessagePayloadSchemas, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to always use parsed message on spy as it can break tests using
toEqual