-
Notifications
You must be signed in to change notification settings - Fork 210
[2025June]Email retry logic #3761
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
base: main
Are you sure you want to change the base?
Conversation
…te/wenjiefan/slice-419737
Issue #576349 is not valid. Please make sure you link an issue that exists, is open and is approved. |
…te/wenjiefan/email-retry-logic
…te/wenjiefan/email-retry-logic
UpdateOutboxStatus(EmailOutbox, EmailOutbox.Status::Failed); | ||
|
||
// if email is not rescheduled, it means it has exceeded the retry limit, stop retrying | ||
if ClientTypeManagement.GetCurrentClientType() = CLIENTTYPE::Background then begin |
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.
@darjoo Can I assume all the emails sent in background belong to the CLIENTTYPE::Background
?
RetryTime: DateTime; | ||
RandomDelay: Integer; | ||
begin | ||
FeatureTelemetry.LogError('', EmailFeatureNameLbl, 'Retry failed email', StrSubstNo(FailedToFindEmailMessageMsg, EmailOutbox."Message Id"), '', Dimensions); |
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 remember Invoke-Miapp
helps to auto fill the Log tag... But I tried it and it doesn't work. @darjoo do you know if we change anything in Invoke-Miapp
? Is it a bug? :)
FeatureTelemetry.LogError('', EmailFeatureNameLbl, 'Retry failed email', StrSubstNo(FailedToFindEmailMessageMsg, EmailOutbox."Message Id"), '', Dimensions); | ||
EmailOutbox.Validate("Retry No.", EmailOutbox."Retry No." + 1); | ||
|
||
if EmailOutbox."Retry No." > 10 then begin |
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.
Now I set maximum retry to 10 times Not sure if it is too much
EmailOutbox.Status := EmailOutbox.Status::Queued; | ||
EmailOutbox.Modify(); | ||
|
||
RandomDelay := Random(5000); // Jitter - Random delay between 0 and 5000 milliseconds (5 seconds) |
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.
The jitter is 5 seconds. Now the concurrency is 10 at most. Do you think the jitter is too short?
EmailOutbox.Modify(); | ||
|
||
RandomDelay := Random(5000); // Jitter - Random delay between 0 and 5000 milliseconds (5 seconds) | ||
RetryTime := CurrentDateTime() + EmailOutbox."Retry No." * 1.5 * 60000 + RandomDelay; // Base interval: 1.5 minutes, plus a random delay of up to 5 seconds |
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 I should use 90000 directly....
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
// ------------------------------------------------------------------------------------------------ | ||
|
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.
Should we add a page for this retry table? When user clicks on the retry No.
, user can jump to the corresponding retry page with the filter on message Id.
trigger OnValidate() | ||
begin | ||
if "Concurrency Limit" > 10 then | ||
Error('The maximum value for Concurrency Limit is 10. Please set a lower value.'); |
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.
Make the error message to a label then it can be translated
if EmailRateLimit.Get(AccountId, Connector) then | ||
exit(EmailRateLimit."Concurrency Limit"); | ||
|
||
EmailRateLimit."Account Id" := AccountId; |
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.
change to validate
Description = 'The field is marked as internal in order to prevent modifying it from code.'; | ||
} | ||
|
||
field(10; "Date Queued"; DateTime) |
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.
Looks we don't need the DateQueued
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.
DateSending
and DateFailed
should be enough
var | ||
EmailOutbox: Record "Email Outbox"; | ||
begin | ||
EmailOutbox.SetRange(Status, EmailOutbox.Status::Processing); |
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 checked the code, EmailOutbox.Status::Processing
should represent the emails are being sent.
Just for confirmation, EmailOutbox.Status::Queued
should not be included here because queued email has not been processed.
Summary
Work Item(s)
Fixes AB#576349