-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use async qute templating for email notifications #1969
Conversation
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.
Hey @barreiro, even if I am not that sure about There have been reports of IO treads blocked while sending out email notifications.
as I couldn't find any confirmation the emails were the root cause I think this PR looks good anyway.
I just left a minor comment, which is more a question.
@@ -47,7 +49,7 @@ public class EmailPlugin implements NotificationPlugin { | |||
@Inject | |||
ReactiveMailer mailer; | |||
|
|||
private final Logger log = Logger.getLogger(getClass()); |
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.
Is there a reason why you switched from a custom logger in favor of the default io.quarkus.logging.Log
?
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.
@barreiro please can you answer this question?
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.
Quarkus simplified logging is the idiomatic (and recommended) way to log in quarkus.
It does not change the functionality: the Logger
will be created for you and the static calls will be routed to that instance.
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.
Makes sense, then I think we can convert all the other loggers to this one to be consistent across the whole codebase. I will create a "good first issue" to keep track of this :)
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.
Created #1993
There have been reports of IO treads blocked while sending out email notifications.
Since horreum uses the async email extension, the other possible cause is qute templating.