Skip to content

Conversation

@wojciechUrbanski
Copy link
Contributor

Fixes #386.

In real SQS, when message is moved to DLQ the SentTimestamp and ApproximateFirstReceiveTimestamp are not preserved.
In ElasticMQ, both of attributes were initialised to new values. Once we were moving message to DLQ, we were treating it as completely new message, thus we were initialising attributes to default values.

Fix assumes, that message that is being sent to DLQ is not a new message. We are treating it as already existing one with filled values that should be copied to DLQ.

…f message once it is moved to dead letters queue
receiveMessages(visibilityTimeout, count, receiveRequestAttemptId)
case DeleteMessage(deliveryReceipt) => deleteMessage(deliveryReceipt)
case LookupMessage(messageId) => messageQueue.byId.get(messageId.id).map(_.toMessageData)
case MoveMessageToDLQ(message) => moveToDeadLetterQueue(message)
Copy link
Member

Choose a reason for hiding this comment

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

maybe simply: MoveMessage. Here the fact that we're moving to the DLQ is not relevant (it's relevant at the time when we make the decision, whether to create a new message or move it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, changed to MoveMessage

if (queueData.isFifo) {
// Ensure a message with the same deduplication id is not on the queue already. If the message is already on the
// queue do nothing.
// TODO: A message dedup id should be checked up to 5 mins after it has been received. If it has been deleted
Copy link
Member

Choose a reason for hiding this comment

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

so we'd have to maintain a cache of already received message ids (for the last 5 mins)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems so. This comment was already there (in similar function). I have copied it to not forgot about this place once someone will do this TODO.
I have not extracted those functions to one common as I feel that behaviour is quite similar but the differences between input/output parameters are too big.

Copy link
Member

Choose a reason for hiding this comment

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

ok :) We'll get back to these 5 minutes in #354 ;)

@adamw adamw merged commit 316277d into master Oct 16, 2020
@mergify mergify bot deleted the propagate-message-data-to-dlq branch October 16, 2020 11:57
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.

SentTimestamp & ApproximateFirstReceiveTimestamp for the message not carried over to DLQ

3 participants