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

asserting dead-letter exchange and queue #68

Merged
merged 11 commits into from
May 16, 2023

Conversation

ujwal-setlur
Copy link
Contributor

This is a PR to assert the dead-letter exchange and queue in the amp adapter, to address #67

The PR asserts the exchange and queue during subscription of the first channel. This was tested by throwing an exception in the channel handler and checking the dead-letter queue on RabbitMQ.

@icebob icebob requested review from AndreMaz and icebob March 26, 2023 12:33
Copy link
Member

@AndreMaz AndreMaz left a comment

Choose a reason for hiding this comment

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

Hi @ujwal-setlur Thank you for the PR. I've checked and left some comments

Can you please add a test for this? It's only necessary for the AMQP (other adapters create queues automatically)

src/adapters/amqp.js Show resolved Hide resolved

// assert dead letter queue
this.logger.debug(`Asserting queue '${chan.deadLettering.queueName}'`);
await this.channel.assertQueue(chan.deadLettering.queueName, queueOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

await this.channel.assertQueue(chan.deadLettering.queueName, _.defaultsDeep(
					{},
					chan.deadLettering ? chan.deadLettering.queueOptions : {},
					this.opts.queueOptions
				));
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -434,22 +468,10 @@ class AmqpAdapter extends BaseAdapter {
* @param {Object} msg
*/
async moveToDeadLetter(chan, msg) {
Copy link
Member

Choose a reason for hiding this comment

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

All the adapters have the HEADER_ORIGINAL_CHANNEL and HEADER_ORIGINAL_GROUP in headers. Removing this logic is something that we would like to avoid in here.

Can you please revert to the original logic?

Copy link
Contributor Author

@ujwal-setlur ujwal-setlur Mar 29, 2023

Choose a reason for hiding this comment

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

@AndreMaz, OK. The reason I did that is that at high message rates, the republish can get a buffer full, and it will throw an error then which then terminates the process. That's not good. At best, it should throw away the message. I will change it to that. Let me know if that's OK.

@ujwal-setlur
Copy link
Contributor Author

@AndreMaz Thanks for the comments. I will review.

@ujwal-setlur
Copy link
Contributor Author

@AndreMaz I have addressed comments. I will add a test as well.

@icebob icebob merged commit b8ca904 into moleculerjs:master May 16, 2023
4 checks passed
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.

None yet

3 participants