Outbox cleanup circuit breaker #242
Conversation
MarcinHoppe
changed the title
…e and used it in OutboxCleaner as that also does other initialization.
MarcinHoppe
force-pushed
the
outboxcleaner-circuitbreaker
branch
from
d771deb to
1987bc2
Compare
|
I rebased this PR on top of develop. |
|
@ramonsmits @Particular/nhibernate-persistence-maintainers Can we use the value that is set using https://docs.particular.net/nservicebus/recoverability/critical-exception-for-timeout-outages It also applies to the persister, so it would make sense to have the circuit breaker behave in the same way for the Timeouts and well as the Outbox. |
|
We could rename the method to not include the Timeouts part in core 6.1 (obsolete with a warning). |
Format code
|
@ramonsmits @Particular/nhibernate-persistence-maintainers I think this PR is good to go, assuming that we want to configure the circuit breaker via AppSettings. Please chime in here. |
MarcinHoppe
changed the title
@MarcinHoppe I intentionally didn't as these are two independent features that would then share implicit schema. Also, the timeout storage is only applicable if the transport doesn't support timeouts natively. |
|
Also, timeouts and Oubox can use two different persisters, so it wouldn't always be good to share the circuit breaker timeout. I think the current API (AppConfig) is good enough and consistent with other Outbox configuration parameters. |
|
@SzymonPobiega @HEskandari Can you give this PR a look and merge? |
| @@ -36,6 +37,25 @@ protected override Task OnStart(IMessageSession busSession) | |||
| throw new Exception("Invalid value in \"NServiceBus/Outbox/NHibernate/FrequencyToRunDeduplicationDataCleanup\" AppSetting. Please ensure it is a TimeSpan."); | |||
| } | |||
|
|
|||
| var key = "NServiceBus/Outbox/NHibernate/TimeToWaitBeforeTriggeringCriticalErrorWhenCleanupTaskFails"; | |||
There was a problem hiding this comment.
I actually inlined this variable to be consistent with how other configuration keys are handled. Key values seem to be descriptive and only used locally in this file. Since they don't change often (doco), encapsulating them brings little value.
Agree?
|
|
||
| if (configValue == null) | ||
| { | ||
| timeToWaitBeforeTriggeringCriticalError = TimeSpan.FromMinutes(2); |
There was a problem hiding this comment.
Extract TimeSpan.FromMinutes(2) as a readonly field to give it more visibility?
There was a problem hiding this comment.
Extracted this and all the other TimeSpans in this file into separate static readonly fields.
|
Few minor things. Otherwise LGTM. |
|
@ramonsmits @MarcinHoppe ping. |
| return; | ||
| } | ||
|
|
||
| timer.Change(System.Threading.Timeout.Infinite, System.Threading.Timeout.Infinite); |
There was a problem hiding this comment.
This is a bit hard to read and capture the intent. Would it be a good idea to have a properly named/intent capturing private method? This is subjective of course so feel free to ignore if you disagree.
|
|
||
| if (newValue == 1) | ||
| { | ||
| timer.Change(timeToWaitBeforeTriggering, NoPeriodicTriggering); |
| TimeSpan timeToWaitBeforeTriggering; | ||
| Action<Exception> triggerAction; | ||
|
|
||
| static TimeSpan NoPeriodicTriggering = TimeSpan.FromMilliseconds(-1); |
There was a problem hiding this comment.
Isn't this the same as Timeout.Infinite?
| } | ||
|
|
||
| long failureCount; | ||
| Exception lastException; |
There was a problem hiding this comment.
Is the last exception enough? or should we go with an AggregateException instead? That'd require a bit more code to ensure exceptions are aggregated correctly and cleared when successful.
Inline config key names
|
@HEskandari The circuit breaker code has been copied verbatim from the core. Do we want to impove upon the code and risk these two copies getting out of sync? I'd rather do a PR in the core and then copy what they come up with. Thoughts? |
|
@MarcinHoppe fair enough. |
|
@HEskandari OK. Can I dismiss your request for changes? @SzymonPobiega If you are OK with my latest changes, this is ready for merge (build is green). |
|
@MarcinHoppe done. |
* Imported RepeatedFailuresOverTimeCircuitBreaker, removed unneeded code and used it in OutboxCleaner as that also does other initialization. * Simplify critical error message * Format code * Extract time parametes into constants
Quick draft to resolve #241
I have put the circuit breaker creation in the cleaner as that already contains configuration. In core circuitbreakers are created in the feature which seems to make more sense but that felt strange to do here unless that configuration value retrieval code is moved to the feature too.