This repository has been archived by the owner on Nov 27, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task Supervisor for message consumers #179
Task Supervisor for message consumers #179
Changes from 14 commits
ccc0367
994625f
c360540
db0b2aa
d5698e5
fb685b8
2f3313e
0996fb5
ccb0896
4eb7ef1
9ce9184
7d016ca
6a28687
59b3e91
0b8298c
073078c
bc2397a
e14bfde
72d45ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
👍
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.
Lovely!
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.
What happens in this case to the message? Will it be rejected/nacked?
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.
Good call....we would probably want some sort of configuration so the user can decide what happens to the message if the task fails. Perhaps we also provide the option to send the message to the deadletter exchange. Also on that same note, we may also want some configuration around how long the Tasks take to complete. Currently it is set to the
async_nolink
value of 5 seconds. Thoughts?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.
Maybe you should be able to set a callback to deal with the message? But I think the sensible default here would be to nack the message.
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.
I like the callback, smth like:
handle_error(reason, state)
. Then in this callback user has all the power to ack, reject or requeue.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.
btw, are we capturing here also task timeouts?
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.
My latest commit adds the ability to configure a timeout for tasks. It was a little tricky to add that in given that Task.Supervisor does not provide that functionality for
async_nolink
, but I think I have a good implementation in place to handle this functionality. I misspoke earlier in regards to the 5 second timeout...that is forasync_stream
andasync_stream_nolink
.As for a custom callback to handle the failure, are you thinking about rewriting the consumer module to a macro so that we can leverage
defoverridable
if the user does not want to use the default error handler? Or wouldhandle_error
be a required behaviour callback that needs to be implemented by the user?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.
@akoutmos I would prefer to avoid rewriting consumer module to a macro at this stage (just to limit the scope of changes).
Since these changes will be released as a major version, requiring error callback to be implemented should be fine, right? @vorce what do you think?
We could also consider skipping error callback and just reject the messages. The problem here is that users might skip dead-letter configuration for their consumers.
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.
@mkorszun @akoutmos Hm hm. Yeah I think making each consumer implement a callback is OK. At least then we encourage users to think about this case. We can have a good implementation in the example consumer that logs and nacks the message.
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.
Sounds good. I'll add an additional callback
handle_error
that will be triggered on task exception or task timeout along with an example or 2. I agree that this is probably the way to go so that gen_rmq does not impose any assumptions upon the user. They can deal with the error the way that best fits their needs.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.
Awesome, and thanks again for all the work @akoutmos