Add email retry to all email events #37408
Merged
Conversation
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
qwef
added
the
backport
qwef
changed the title
dpsutton
reviewed
Jan 9, 2024
src/metabase/email.clj
Outdated
| :multiplier (email-retry-multiplier) | ||
| :randomization-factor (email-retry-randomization-factor) | ||
| :max-interval-millis (email-retry-max-interval-millis)} | ||
| (or config/is-dev? config/is-test?) (assoc :max-attempts 1))) |
There was a problem hiding this comment.
why do we set max attempts to 1 in dev?
There was a problem hiding this comment.
I assume for testing purposes, I used it a bit to test the retry logic but can remove it.
dpsutton
reviewed
Jan 9, 2024
src/metabase/email.clj
Outdated
| [& args] | ||
| (try | ||
| (when-not @retry-state | ||
| (compare-and-set! retry-state nil (make-retry-state))) |
There was a problem hiding this comment.
this feels weird to me. We could likely send emails from lots of threads. Having a single global shared retry state feels quite strange.
There was a problem hiding this comment.
Ah right that was also an issue with the old pulse retry logic code, I'll take a look into creating different retry states for different events
noahmoss
requested changes
Jan 11, 2024
There was a problem hiding this comment.
Aside from nits, my main concern is about scrapping the mechanism that stores a single Retry object which is reused for each email send. I don't know if it would cause issues, but it's an easy optimization to not be recreating a Retry each time if the settings haven't changed.
Let me know if there's a reason for getting rid of that that I'm not aware of.
Edit: Thought about it again, I guess this is fine. (See comment below)
Comment on lines
182
to
+186
| (email/send-message! | ||
| :subject (trs "[{0}] Password Reset Request" (app-name-trs)) | ||
| :recipients [email] | ||
| :message-type :html | ||
| :message message-body))) | ||
| {:subject (trs "[{0}] Password Reset Request" (app-name-trs)) | ||
| :recipients [email] | ||
| :message-type :html | ||
| :message message-body}))) |
There was a problem hiding this comment.
Did this need to change to take the email settings as a map instead of as keyword args?
I see other calls of send-message! are still using keyword args. So I'm wondering if those need to be changed too. Plus the docstring for send-message!.
Or, otherwise, why not keep using the keyword args for this call site?
There was a problem hiding this comment.
I think both work actually since [& {:as msg-args}] will make it into a map.
src/metabase/util/retry.clj
Outdated
| :multiplier (retry-multiplier) | ||
| :randomization-factor (retry-randomization-factor) | ||
| :max-interval-millis (retry-max-interval-millis)} | ||
| (or config/is-dev? config/is-test?) (assoc :max-attempts 1))) |
There was a problem hiding this comment.
Is this necessary? I wonder if tests should just override max-attempts if they explicitly don't want the retry.
It's probably not a big deal, but I think we should be very intentional about anything that changes behavior between dev and prod in case it makes issues trickier to reproduce.
There was a problem hiding this comment.
Yea makes sense, will remove.
| :type :integer | ||
| :default 30000) | ||
|
|
||
| (defn- retry-configuration [] |
There was a problem hiding this comment.
It's a little weird that we have defaults defined for all these settings, but random-exponential-backoff-retry also has defaults that are used if options aren't passed in, and they have different values. I'm assuming that second set of defaults is never being used, so can we remove those and require the config to be passed in for all values?
There was a problem hiding this comment.
Yea sounds good, my bad I didn't look too closely into the random-exponential-backoff-retry code
Comment on lines
134
to
137
| retry/decorate (fn [f] | ||
| (fn [& args] | ||
| (let [callable (reify Callable (call [_] (apply f args)))] | ||
| (.call (Retry/decorateCallable test-retry callable)))))] |
There was a problem hiding this comment.
Can you pull this out as a test-retry-decorate-fn or something, similar to fake-inbox-email-fn? It's a bit verbose to repeat it in the setup for every test.
|
Oh, just saw the comment from Dan above. Just looked at the docs for I guess you're right that we need a separate Other feedback still applies though—mostly nits. |
escherize
approved these changes
Jan 11, 2024
src/metabase/email.clj
Outdated
| (defn send-email-retrying! | ||
| "Like [[send-message-or-throw!]] but retries sending on errors according to the retry settings." | ||
| [& args] | ||
| (apply (retry/decorate send-message-or-throw!) args)) |
There was a problem hiding this comment.
I think, since send-message-or-throw! takes one arg, it's necessary to use [& args and apply here.
Also it seems like it's hiding the schema from dev-users (I guess just 1 layer of indirection), but if send-message-or-throw! is only called through send-email-retrtying! now, then I think it should be a mu/defn taking EmailMessage, and send-message-or-throw! can be a plain defn. The principle here is to use mu/defn on "api boundaries".
Comment on lines
+183
to
+186
| {:subject (trs "[{0}] Password Reset Request" (app-name-trs)) | ||
| :recipients [email] | ||
| :message-type :html | ||
| :message message-body}))) |
There was a problem hiding this comment.
Thanks, the style you changed it to is better. (data good, seqential key args bad)
There was a problem hiding this comment.
Sounds good, will change the comment and rest of the functions to match this
Not a big fan of creating a shared state for just the config, I don't think the performance gain would be worth it to keep the config always in memory. |
noahmoss
approved these changes
Jan 11, 2024
|
|
@qwef Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
github-actions bot
pushed a commit
that referenced
this pull request
Jan 12, 2024
* add retry to password reset * fix error case * add to all emails * fix misc stuff * fix test * fix test * fix linter * fix retrying framework * fix linter * fix e2e test * add comment * adress comments * fix linter * fix linter * fix test * fix test * fix test * fix test * fix test * fix e2e test * fix e2e test * fix e2e test * fix test * fix e2e test and unit tests * remove paren * fix linter * fix e2e test * fix e2e test * finally actually fix e2e test
metabase-bot bot
added a commit
that referenced
this pull request
Jan 12, 2024
* add retry to password reset * fix error case * add to all emails * fix misc stuff * fix test * fix test * fix linter * fix retrying framework * fix linter * fix e2e test * add comment * adress comments * fix linter * fix linter * fix test * fix test * fix test * fix test * fix test * fix e2e test * fix e2e test * fix e2e test * fix test * fix e2e test and unit tests * remove paren * fix linter * fix e2e test * fix e2e test * finally actually fix e2e test Co-authored-by: Jerry Huang <34140255+qwef@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #23030
Description