-
Notifications
You must be signed in to change notification settings - Fork 671
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
Introduced SMTP notification #5535
Merged
wild-endeavor
merged 33 commits into
flyteorg:master
from
mercedes-benz:smtp-notifications
Sep 16, 2024
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
b11f2cc
Introduced SMTP notification
robert-ulbrich-mercedes-benz 3296a1a
Fixing lint issues
robert-ulbrich-mercedes-benz d6bc1a4
Fixing tests
robert-ulbrich-mercedes-benz a740c8e
Fixing lint
robert-ulbrich-mercedes-benz 4be6a0e
Fixing lint
robert-ulbrich-mercedes-benz 4353452
Fix linting issues
robert-ulbrich-mercedes-benz 9aa1b3e
Merge branch 'refs/heads/master' into smtp-notifications
robert-ulbrich-mercedes-benz 3d74ea9
Merge branch 'refs/heads/master' into smtp-notifications
robert-ulbrich-mercedes-benz cada9d8
Fix go mod
robert-ulbrich-mercedes-benz fccc16a
Merge branch 'refs/heads/master' into smtp-notifications
robert-ulbrich-mercedes-benz 73bb305
Fixing location of no sec comment
robert-ulbrich-mercedes-benz c7901bc
Removing unused import
robert-ulbrich-mercedes-benz b265304
Running gci for file
robert-ulbrich-mercedes-benz eda4d3e
Implemented connection reuse for smtp emailer
robert-ulbrich-mercedes-benz 445b979
Introduced SMTP notification
robert-ulbrich-mercedes-benz a720425
Fixing lint issues
robert-ulbrich-mercedes-benz 0bf9654
Fixing tests
robert-ulbrich-mercedes-benz f513352
Fixing lint
robert-ulbrich-mercedes-benz 1a0de4a
Fixing lint
robert-ulbrich-mercedes-benz dc88a99
Fix linting issues
robert-ulbrich-mercedes-benz b54a932
Fix go mod
robert-ulbrich-mercedes-benz 5eac9ea
Fixing location of no sec comment
robert-ulbrich-mercedes-benz 0926604
Removing unused import
robert-ulbrich-mercedes-benz 9d72fac
Running gci for file
robert-ulbrich-mercedes-benz d995db7
Implemented connection reuse for smtp emailer
robert-ulbrich-mercedes-benz f04c0fd
Merge remote-tracking branch 'origin/smtp-notifications' into smtp-no…
robert-ulbrich-mercedes-benz 5da05e1
Adapting interface of smtp_emailer
robert-ulbrich-mercedes-benz 1050f4b
Fixing linter issue
robert-ulbrich-mercedes-benz 6d0e4bf
Adding unit tests
robert-ulbrich-mercedes-benz 39369ba
Merge branch 'refs/heads/master' into smtp-notifications
robert-ulbrich-mercedes-benz 8be8c46
Applying gci
robert-ulbrich-mercedes-benz 64f3333
Faking mockery generation
robert-ulbrich-mercedes-benz 50ce320
Using mocker 1.0.1
robert-ulbrich-mercedes-benz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Implemented connection reuse for smtp emailer
Signed-off-by: Ulbrich Robert <robert.ulbrich@mercedes-benz.com>
- Loading branch information
commit d995db797796a5bab865ae1ccf073700798bd552
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Just to confirm, this is the intended behavior? If admin needs to send a thousand emails, it has to make a new client each time?
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.
Yes, this is intended behavior. When using the built-in email client of Go, most examples you find work that way. Also with the integration into the Notifications processor this is the simplest approach. Establishing a connection does not seem to be a very complex undertaking when looking at the source code of the email client.
I agree that keeping the connection is more efficient. But it also makes the code more complex, because the connection state needs to be maintained over an instance - so whenever an email is meant to be sent, it first needs to be checked if the connection is still alive.
If you want me to, I can refactor it to only open a connection once to the SMTP server.
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 just checked again the internal implementation of Golang's smtp client: It appears that the client implementation seems to be not multi-thread-safe. Every client instance keeps an internal state between function calls - so when reusing the same client while sending an email for writing another mail can lead to weird behavior.
My assumption is that there is only a single email processor per Flyte Admin instance. That means that there should be no concurrent email processing on any Flyte Admin instance, which makes it safe to reuse an existing Flyste client.