FallbackMailer: shuffle mailers with given probability

[JanTvrdik]

• bug fix? no
• new feature? yes
• BC break? no
• doc PR: would be nice to have

Intended to periodically test the other mailers.
I'm not sure whether default $shuffleProbability should be 0.0 or sth like 0.001 (would be a minor BC break).

[JanTvrdik]

@janedbal Can you elaborate?

[janedbal]

@JanTvrdik I don't like the intention. If you want to periodically test other mailers, include it in your CI or cron task. IMHO fallback mailer should be used in "emergency" cases, then you can for example track usage of the fallback mailer to measure reliability of your primary mailer.

[JanTvrdik]

@janedbal Production SMTP servers are rarely tested on CI (because CI does not have access to production credentials). Also being able to send mail from CI does not mean you will be able to send mail from production (different firewall configuration…).

Reliability can be tracked via onFailure event. The default probability is zero, meaning nothing changes unless you explicitly asked for random shuffling to be enabled (although this is up for discussion).

[hrach]

Well, adding such functionality into a constructor isn't good. It also violates SRP. Introducing a "ShuffleMailer" may be the right way.

[JanTvrdik]

Introducing a "ShuffleMailer" may be the right way.

Feel a lot like overengineering. This is three lines of code. Creating new class would just make this a lot harder to use.

BTW: FallbackMailer intentionally already violates SRP (the „correct“ approach would be to split it to RetryingMailer and FallbackMailer).

[Majkl578]

It also violates SRP. Introducing a "ShuffleMailer" may be the right way.

💯

[hrach]

Feel a lot like overengineering.

Agreed. Maybe the question is if a such unique functionality should be included in the framework.

BTW: FallbackMailer intentionally already violates SRP

This argument is based on the actual implementation, but the general intention is in your case totally different - the current Fallback mailer tries to send the mail in any case, tries to solve some failures and outages. Your extension actually doesn't increase such possibility of successfull send at all, but it introduces possibility to test the infrastructure. That's another usecase, therefore the logic should be IMO separated.

[Majkl578]

Intended to periodically test the other mailers.

This clearly doesn't sound like something that should be done in production. If your mailers are unreliable, you should test them in different manner, i.e. by your monitoring, not by randomly using them in production (or rather not use them in production at all)...

FallbackMailer should probably be renamed to FallbackRetryingAndWaitingAndTestingByShufflingMailer.

Seriously, shuffling of the mailers has absolutely no relation to fallback mailing. Also it's just a rare use case imho (which you can easily implement in user-land).

[JanTvrdik]

This clearly doesn't sound like something that should be done in production

I disagree, imho production is the only place this can be accurately done from. Yes, you can use monitoring to test whether the mail servers are running, but that does not mean that the application will be able to actually send mails.

FallbackMailer should probably be renamed to FallbackRetryingAndWaitingAndTestingByShufflingMailer

Maybe MoreReliableMailer would be better name as all those 3 responsibilities are intended to increase reliability of mail delivery, but FallbackMailer looks good enough to me.

[dg]

Because it instantly modifies the input argument and because it is tricky (or hard to explain, reuse, whatever), I think it can be better solved in the config:

mail.mailer: Nette\Mail\FailbackMailer(MyTools::suffleMailers([@onemailer, @secondmailer]))

function MyTools::suffleMailers(array $mailers, $shuffleProbability = 0.01)
{
   if ($shuffleProbability > 0.0 && mt_rand() / mt_getrandmax() < $shuffleProbability) {
		shuffle($mailers);
   }
   return $mailers;
}

[JanTvrdik]

Because it instantly modifies the input argument

We can move the actual shuffle to send() + decrease the probability to zero to avoid shuffling for each send() call.