-
-
Notifications
You must be signed in to change notification settings - Fork 614
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
Notify-mailer: save and reuse resolved email addresses #7979
base: main
Are you sure you want to change the base?
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.
New flags LGTM. Just a couple tiny comments about comments, and one big piece of feedback about how we use the data from the loaded file to actually save ourselves time.
err = os.WriteFile(filename, jsonData, 0644) | ||
return err |
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.
err = os.WriteFile(filename, jsonData, 0644) | |
return err | |
return os.WriteFile(filename, jsonData, 0644) |
@@ -153,6 +166,16 @@ func (m *mailer) run(ctx context.Context) error { | |||
m.log.Infof("Address %q was associated with the most recipients (%d)", | |||
mostRecipients, mostRecipientsLen) | |||
|
|||
// If saveEmailsTo set but readEmailsFrom not, write the map of resolved |
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.
// If saveEmailsTo set but readEmailsFrom not, write the map of resolved | |
// If saveEmailsTo is set but readEmailsFrom is not, write the map of resolved |
// `recipient`s that resolve to that email address. If readEmailsMap isn't nil: | ||
// a) if saveEmailsTo unset, then return readinsteadEmailsMap | ||
// b) otherwise, resolve addresses that aren't in readEmailsMap |
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.
// `recipient`s that resolve to that email address. If readEmailsMap isn't nil: | |
// a) if saveEmailsTo unset, then return readinsteadEmailsMap | |
// b) otherwise, resolve addresses that aren't in readEmailsMap | |
// `recipient`s that resolve to that email address. If readEmailsMap is | |
// populated and we're not saving emails into saveEmailsTo, just return | |
// the loaded map. |
{"Data":{"date":"2018-11-21","domainName":"example.com"}}, | ||
{"Data":{"date":"2018-11-22","domainName":"example.net"}} | ||
] | ||
} |
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.
This file doesn't have a terminating newline character.
[ | ||
{"Data":{"date":"2018-11-21"}} | ||
] | ||
} |
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.
This file doesn't have a terminating newline character.
if _, ok := m.readEmailsMap[parsed.Address]; !ok { | ||
result[parsed.Address] = append(result[parsed.Address], recipient) | ||
} |
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.
Hmm. So while the end result here is what we want -- only updating the map for addresses that we haven't already processed -- this stanza is too late to save us from actually doing extra work. We're still doing all of the database lookups for every recipient, we're just dropping the result on the floor if it turns out we've looked this one up before.
I think this needs to happen earlier. When we read the emails map from disk, we should also keep track of every regID seen in that map. And then we can add something like
_, present := m.cachedRecipients[recipient.id]
if present {
continue
}
between lines 286 and 287 of this function, to actually save ourselves work.
Add
saveEmailsTo
andreadEmailsFrom
optionals flags to notify-mailer to enable saving and reusing work to resolve email addresses.saveEmailsTo
is the filename of where to save resolved address/recipient datareadEmailsFrom
is the filename of where to read resolved address/recipient data.New use cases:
saveEmailsTo
flag and notify-mailer will run and save resolved address/recipient data to the specified file.readEmailsFrom
flag and notify-mailer will extract the saved recipient data to send emails, instead of redoing the work to resolve addresses.readEmailsFrom
andsaveEmailsTo
flags. Notify-mailer will read the addresses inreadEmailsFrom
and then resolve addresses from the input recipients file if the address is NOT in readEmailsFrom. After resolving and sending to the new recipients, the combined set of addresses sent by the current run and seen inreadEmailsFrom
are saved to thesaveEmailsTo
file.Important Note: doing a batch send (case 3) WILL NOT WORK entirely correctly if the email body includes recipient-specific data (e.g. embedding recipient's certificate domain names). Only use both
saveEmailsTo
andreadEmailsFrom
flags if email body is exactly the same regardless of recipient.