Failure alerts

[mpchadwick]

This is the feature that I outlined in #3.

Since configuration for this module is set up through local.xml I set this up that way as well, rather than introducing an area in the system configuration. E.g.

<config>
  <global>
    <resources>
      <aoe_dbretry_alert_threshold>10</aoe_dbretry_alert_threshold>
      <aoe_dbretry_alert_recipients>myemail@example.com</aoe_dbretry_alert_recipients>

I've tried to keep this as lightweight as possible, but it does require introducing a new cron job. I understand if you feel this is out of scope for this project, but it is a requirement for one of our clients, so I figured I'd see if you would incorporate it upstream.

[mpchadwick]

Wondering if you had a chance to look at this? I see the build failed

The closing brace for the class must go on the next line after the body (PSR2.Classes.ClassDeclaration.CloseBraceAfterBody)

I can fix that, but wondering this is something you'd consider in scope of the project or if we should look to locally incorporate this change since it's a requirement for one of our clients

[LeeSaferite]

Sorry, I meant to reply when you added this but I got sidetracked and forgot to circle around.

First, I would say not to use underscores in method names. This is a very old convention from when PHP didn't support visibility of methods. The only reason PHPCS didn't complain about those is I had to disable the rule since one of the methods we replaced used an underscore.

Second, this fails totally on at least one site I run that never writes log files to disk. I see that the email just warns of the error count while the log has full errors. Perhaps we can disconnect the error count by storing it in the cache. That breaks the error count reporting away from physical errors in a log file os disk.

Third, and more important, I wonder if this is really something we should add directly to this module. I get the reasoning, but is there a was to split this away and still have it functional? I ask because this is a VERY simple module and this is just adding complexity.

Anyway, if you can address #1 and #2 then I'll merge the code. Think about #3 if you can as well.

[mpchadwick]

Your third question above is the exact question I've been asking myself. This is a requirement for our client, but could easily be argued out of scope for this module (actually I kind of think that it is). Maybe we could just dispatch events

Mage::dispatchEvent('aoe_dbretry_error', array('iteration' => $try, 'retry' => bool, 'exception' => $e))

Retry will be false in the place where I currently have the "unrecoverable error" logging happening indicating that it will not retry anymore.

This will allow others (such as myself) to hook in where needed

[philwinkle]

@LeeSaferite @mpchadwick fwiw I would advocate for the dispatch and be done with it. It allows 3rd party functionality while keeping the module single-purpose.

[LeeSaferite]

I thought of the dispatch, but I worried about timing. I think the DB code is called before dispatchEvent is valid. I'll have to check.

[mpchadwick]

@LeeSaferite tested locally and Mage::dispatchEvent() is working fine.

I've used these methods for simulating deadlocks.

Since the module already uses Mage::log() I'd assume that any of the Mage static methods could be called.

I'm closing this and will open a new PR using Mage::dispatchEvent()