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

⚡ Add manageable acknowledge for services #2646

Closed
wants to merge 1 commit into from

Conversation

m2scared
Copy link
Contributor

@m2scared m2scared commented Jan 6, 2022

Add the option to acknowledge or not acknowledge messages coming from
some trigger services.

The current implementation reuses sendResponse logic from node
RespondToWebhook.

@m2scared
Copy link
Contributor Author

m2scared commented Jan 6, 2022

Hi,

This is an initial idea for adding manageable acknowledge for Trigger nodes such as MQTT, RabbitMQ, Kafka and etc. There is room for improvement since it is using sendResponse logic from the Webhook family nodes.

Maybe this should have its own sendResponse? Is it handling properly the error/shutdown path?

I would love to hear what could be improved.

Add the option to acknowledge or not acknowledge messages coming from
some trigger services.

The current implementation reuses `sendResponse` logic from node
`RespondToWebhook`.
@ivov ivov added node/improvement New feature or request community Authored by a community member labels Jan 7, 2022
@m2scared
Copy link
Contributor Author

Hi @Joffcom,

If you have time, I would love to hear your thoughts on this one. Thanks.

@m2scared
Copy link
Contributor Author

m2scared commented Apr 5, 2022

Hi @nivb06,

What do you think about this one?

@Joffcom
Copy link
Member

Joffcom commented Apr 5, 2022

Hey @m2scared,

I like the idea, I am not sure if "Respond to trigger" is too generic though as it wouldn't do anything for any of the other trigger nodes at the moment (apart from webhooks).

Would it maybe be better as an option in the RabbitMQ node for now if possible? Do you also have an example of when you may use this?

@m2scared
Copy link
Contributor Author

m2scared commented Apr 7, 2022

Hey @m2scared,

Hi @Joffcom,

I like the idea, I am not sure if "Respond to trigger" is too generic though as it wouldn't do anything for any of the other trigger nodes at the moment (apart from webhooks).

Understood. I used this name because it can be reused in other "queue" nodes that support NACK such as Kafka.

Would it maybe be better as an option in the RabbitMQ node for now if possible? Do you also have an example of when you may use this?

I agree. My use case is: the event should be acknowledge only if the workflow succeeds. Currently this is not possible with n8n as far as I know. Is there any other way to archive this?

I will change the patch only for RabbitMQ.

@Joffcom
Copy link
Member

Joffcom commented Apr 7, 2022

No other way I can think of to handle it at the moment, Makes sense that you would only want to ack a success as well. In the past when implementing a logging system for file transfers with an MQ broker (Websphere I think it was) we would just post a sucess message to a queue / topic and include the original MQ ID as a header but I guess something like that would depend on how the interacting systems are working together.

@m2scared
Copy link
Contributor Author

m2scared commented Apr 7, 2022

No other way I can think of to handle it at the moment, Makes sense that you would only want to ack a success as well. In the past when implementing a logging system for file transfers with an MQ broker (Websphere I think it was) we would just post a sucess message to a queue / topic and include the original MQ ID as a header but I guess something like that would depend on how the interacting systems are working together.

I am not quite sure if I understood. Do you have examples? Code?

It seems possible to do since there is a similar behavior with 'webhook' and 'respond to webhool'. My current implementation uses createDeferredPromise and works as expected. I don't know if it will work with worker n8n nodes.

@Joffcom
Copy link
Member

Joffcom commented Apr 7, 2022

Sadly nothing I can share for the old MQ project as it was done using other systems for an old customer, If I get a chance I will see what I can remember but chances are as you are already doing something with MQ it may not be that helpful. It all depends on what your MQ process currently is, So we were getting an MQ message with transfer info in it and would then connect to a server to download or upload a file then anything that happened would be logged to a queue and the MQ ID was passed along the MQ messages. They then had this neat web based system which would show them everything when then entered the ID.

@michael-radency
Copy link
Contributor

Hello @m2scared , thank you for your contribution, as for now it seems that RespondToTrigger node is redundant and too tied to RabbitMQTrigger node, as this node only hold boolean value it probably can be replaced with an option in RabbitMQ trigger node.

Would be great if you can share workflow with setup where you are using this node, as it will clarify use case and gives possibility of alternative solution without necessity to add additional node.

RabbitMQTrigger uses amqplib, here is documentation that can be useful.

@nivbe06
Copy link
Contributor

nivbe06 commented May 4, 2022

Hi @m2scared, I will be closing this as of @michael-radency's comments.
Feel free to reopen this or open a new PR based on Michael's comments
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants