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

Expose API for ACK, NACK and Requeue operations #32

Closed
kjetilhp opened this issue Apr 2, 2019 · 14 comments · Fixed by #33
Closed

Expose API for ACK, NACK and Requeue operations #32

kjetilhp opened this issue Apr 2, 2019 · 14 comments · Fixed by #33
Assignees
Labels
enhancement New feature or request

Comments

@kjetilhp
Copy link

kjetilhp commented Apr 2, 2019

In a publish/subscribe would it be possible to nack the message if handling fails? or do we have requeue it manually?

@WonderPanda
Copy link
Collaborator

Hey @kjetilhp This is a great question. At the moment the functionality isn't supported but it's been on my mind for a while. What would your preferred API look like for this? Would you prefer to get the actual AMQP message object in the handler so that you can ack or nack it yourself? Or I could introduce a simple set or Error messages that would allow to Nack or Requeue if thrown from the handler?

I have short term plans to move the underlying AMQP code into it's own repository and add some core missing functionality so would love the feedback

@WonderPanda
Copy link
Collaborator

I'm leaning towards an API that looks similar to RawRabbit from the .NET ecosystem:
https://github.com/pardahlman/RawRabbit#ack-nack-reject-and-retry

@kjetilhp
Copy link
Author

kjetilhp commented Apr 2, 2019

That API looks promising, I guess it wouldn't be need for the message obj if it could be handled that way.

@WonderPanda
Copy link
Collaborator

Thanks for the feedback. I'll take a look at getting this integrated into the library tonight. Out of curiosity, are you evaluating using @nestjs-plus/rabbitmq for a personal or work project? I would love to get a feel for who the people considering adoption are so that I can best support the library

@WonderPanda WonderPanda self-assigned this Apr 2, 2019
@WonderPanda WonderPanda added the enhancement New feature or request label Apr 2, 2019
@WonderPanda WonderPanda changed the title question: is it possible to do ack/nack on messages? Expose API for ACK, NACK and Requeue operations Apr 2, 2019
@kjetilhp
Copy link
Author

kjetilhp commented Apr 2, 2019 via email

@WonderPanda
Copy link
Collaborator

That's awesome to hear. Feel free to open other issues if you run into functionality that's missing which you need to move forward

WonderPanda added a commit that referenced this issue Apr 3, 2019
- adds support for explicit nacking of message including option to requeue
- configurable behavior for default error handling in rpc and subscribe
- configurable timeout on a per rpc call basis with configurable default
fallback
- fixed bug where prefetch limit wasn't being applied

fix #32
@WonderPanda
Copy link
Collaborator

WonderPanda commented Apr 3, 2019

Opening #33 to address this issue. This also adds configurable default behavior for how messages should be handled when errors occur as well as a couple bug fixes and tweaks. Things are looking in good shape just want to look at adding some integration tests before it's merged.

WonderPanda added a commit that referenced this issue Apr 3, 2019
- adds support for explicit nacking of message including option to requeue
- configurable behavior for default error handling in rpc and subscribe
- configurable timeout on a per rpc call basis with configurable default
fallback
- fixed bug where prefetch limit wasn't being applied

fix #32
@kjetilhp
Copy link
Author

kjetilhp commented Apr 3, 2019

off-topic, but wouldn't create another issue since it's more of a question
have you considered exposing the connection as a decorator like nestjs-amqp does so we could use this as a publishing client also? second, is there a reason why you use build() and not the typical forRoot(..)/forRootAsync(..), just wondering since I was looking to inject my config service into the build method like other modules I use..

AmqpModule.forRootAsync({
      useFactory: async (configService: ConfigService) => {
        return await configService.rabbitConfig()
      },
      inject: [ConfigService],
    }),

@WonderPanda
Copy link
Collaborator

A decorator shouldn't be necessary, the connection is already set as a provider internally so you can just inject it using AmqpConnection. You can see an example here https://github.com/WonderPanda/nestjs-plus/blob/master/examples/kitchen-sink/src/rabbit-example/messaging/messaging.controller.ts

This was the intended use case as it provides methods for both publishing messages as well as making requests to RPC handlers which will definitely come in handy for apps using Rabbit.

As for the config stuff I'm willing to take a look at changing that up to being the naming convention and usage closer to other modules in the ecosystem. I'll probably do that under a separate ticket for tracking.

@kjetilhp
Copy link
Author

kjetilhp commented Apr 3, 2019

Sorry about that, I found your kitchen sink sample after writing :)

@WonderPanda
Copy link
Collaborator

No worries :)

I created #34 to track the changes to the way the RabbitMQModule gets created

WonderPanda added a commit that referenced this issue Apr 7, 2019
- adds support for explicit nacking of message including option to requeue
- configurable behavior for default error handling in rpc and subscribe
- configurable timeout on a per rpc call basis with configurable default
fallback
- fixed bug where prefetch limit wasn't being applied

fix #32
@WonderPanda
Copy link
Collaborator

@kjetilhp New packages are published. Let me know if you get the chance to try them out and have any more issues or feedback

@kjetilhp
Copy link
Author

Hi, just to be sure, if nothing is returned from the pubsub handler, the message gets acked, and then you have return values Nack() to not ack, and Nack(true) to not ack and requeue?

@WonderPanda
Copy link
Collaborator

It depends on whether or not you're using RPC or PubSub. In both cases the behavior around Nacking is the same. Nack() will just nack the message while Nack(true) will nack and requeue as you've already stated.

Behavior for what is returned from the handler is slightly different. PubSub expects to not need to return any values to the client so if your handler returns a value it's unclear what should be done with this value so currently an exception is thrown:
https://github.com/WonderPanda/nestjs-plus/blob/master/packages/rabbitmq/src/amqp/connection.ts#L121

I'm not committed to the error though could definitely consider just emitting a warning log instead because generally I'd see that as being an issue with the design of your PubSub handler. I'll update the docs around this stuff probably this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants