Generic watchdogs infrastructure #3218
Conversation
SergeyGaluzo
commented
Apr 6, 2023
•
SergeyGaluzo
commented
- Adds watchdogs infra based on lease paradigm.
- Retrofits Defrag to new infra.
- Adds new Cleanup watchdog.
- Adds tests.
SergeyGaluzo
added
New Feature
src/Microsoft.Health.Fhir.SqlServer/Features/Watchdogs/DefragWatchdog.cs
Outdated
Show resolved
Hide resolved
| await _defragWatchdog.StartAsync(stoppingToken); | ||
| await _cleanupEventLogWatchdog.StartAsync(stoppingToken); | ||
|
|
||
| while (true) |
There was a problem hiding this comment.
Is this loop needed? Once the watchdogs are started does this method need to keep running?
There was a problem hiding this comment.
I don't know for sure. It depends on what will happen when background service completes. Will it be GCed together with watchdogs it is holding?
src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/AcquireWatchdogLease.sql
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/AcquireWatchdogLease.sql
Show resolved
Hide resolved
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerWatchdogTests.cs
Show resolved
Hide resolved
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerWatchdogTests.cs
Show resolved
Hide resolved
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerWatchdogTests.cs
Show resolved
Hide resolved
| ( | ||
| Watchdog varchar(100) NOT NULL | ||
| ,LeaseHolder varchar(100) NOT NULL CONSTRAINT DF_WatchdogLeases_LeaseHolder DEFAULT '' | ||
| ,LeaseEndTime datetime NOT NULL CONSTRAINT DF_WatchdogLeases_LeaseEndTime DEFAULT 0 |
There was a problem hiding this comment.
Should timers record when the last instance was executed? This would give you something to resume from is something happened to the original process
There was a problem hiding this comment.
In general case, the state of the abandoned process cannot be determined by its last success date. It might be useful in certain cases, but not generic... Also leasing goes on different intervals than execution
There was a problem hiding this comment.
This doesn't guarantee that a timer will be run at a regular interval though. Timers that are run less often could more likely be impacted by abandoned processes, or be subject to running at more or less frequent intervals than intended
There was a problem hiding this comment.
That is true. Watchdogs with long periods can be affected. I don't think that this behavior requires fixing though. For example for defrag and cleanup watchdogs it does not matter.
|
@brendankowitz I think something is wrong with the metadata validation. This PR has the New Feature tag, but the validation is saying it is missing that type of tag. |
|
I'm adding the enhancement tag as it is the next best fit. |
...osoft.Health.Fhir.SqlServer/Registration/FhirServerBuilderSqlServerRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
