-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Abstract submit and poll operations #19688
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
base: main
Are you sure you want to change the base?
Abstract submit and poll operations #19688
Conversation
…sks to use this service This service will manage operations that require status to be synced between servers (load balanced setup).
This is both async and returns an attempt, which will fail if a rebuild operation is already running.
…on-background operations. Storing an expiration date allows setting different expiration times depending on the type of operation, and whether it is running in the background or not.
… expiration and deletion in `LongRunningOperationRepository.CleanOperations`.
…rm on why a result could not be retrieved
…ons-in-a-LB-friendly-way # Conflicts: # src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs
…round and stale operations
…ons-in-a-LB-friendly-way
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.
Pull Request Overview
This PR abstracts long-running operations into a unified service, replacing direct background-queue usage in key services, and adds infrastructure to persist, track, and clean up those operations.
- Introduce
LongRunningOperationService
and its repository for enqueueing, tracking status, and storing results - Refactor
ContentPublishingService
andDatabaseCacheRebuilder
to use the new service - Add cleanup job, migrations, DI registrations, and comprehensive unit/integration tests
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Umbraco.Core/Services/LongRunningOperationService.cs | Core service to enqueue, run, and track long-running operations |
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LongRunningOperationRepository.cs | Repository persisting operation metadata, status, and results |
src/Umbraco.Core/Services/ContentPublishingService.cs | Refactored branch publish to use LongRunningOperationService |
src/Umbraco.PublishedCache.HybridCache/DatabaseCacheRebuilder.cs | Refactored rebuild logic to use LongRunningOperationService |
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/LongRunningOperationsCleanupJob.cs | Recurring job deleting stale operations after one day |
src/Umbraco.Core/DependencyInjection/UmbracoBuilderExtensions.cs | Register the cleanup job in recurring background jobs |
src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Repositories.cs | Register new ILongRunningOperationRepository |
src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs | Register ILongRunningOperationService |
tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/LongRunningOperationServiceTests.cs | Unit tests for LongRunningOperationService |
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/LongRunningOperationRepositoryTests.cs | Integration tests for repository behavior |
Comments suppressed due to low confidence (2)
src/Umbraco.Core/Services/ILongRunningOperationService.cs:20
- The default value for
allowConcurrentExecution
isfalse
here but istrue
in the implementation; aligning these defaults will prevent surprising behavior when callers omit this argument.
Task<Attempt<Guid, LongRunningOperationEnqueueStatus>> Run(
src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs:193
- [nitpick] There's an extra space before this line relative to others in the block; aligning indentation improves readability and consistency.
builder.Services.AddRecurringBackgroundJob<LongRunningOperationsCleanupJob>();
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.
Looks great so far @lauraneto, really impressive. I've tested out the happy paths and they all work as expected. I know you are still working on a few things but will share the points I've found in the code review now.
/// <summary> | ||
/// Represents a repository for managing long-running operations. | ||
/// </summary> | ||
public interface ILongRunningOperationRepository |
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.
We could, and probably should, use async methods here. I know we don't in many repositories, but in the newer ones - e.g. IWebhookRepository
- we do. I'd suggest reviewing that interface and aligning the methods names, return types and use of async for this one.
Also should align for retrieving collections - so we get paged results.
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.
Adjusted!
Also added Async
as a suffix to the methods, but only in the repository. Should that also be done for the rest?
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.
Yes, I think so. Again I'm only going by looking at what's been done for more recent services and repositories, and I see IWebhookService
has Async
suffixes.
If and when we get to a point where most things you expect to be async are, then you could argue these suffixes are superfluous, but we are a way from that, and so for now I'd suggest just align as close as possible with the newer instances.
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.
Adjusted!
src/Umbraco.Infrastructure/BackgroundJobs/Jobs/LongRunningOperationsCleanupJob.cs
Show resolved
Hide resolved
} | ||
|
||
/// <inheritdoc /> | ||
public TimeSpan Period => TimeSpan.FromMinutes(2); |
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.
Perhaps we should have configuration for some of these options on when the job runs, and in particular how many days back to clean up. See for example CacheInstructionsPruningJob
and HealthCheckNotifierJob
.
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.
Adjusted in 7c572e5 but not sure if any other changes are needed.
private readonly TimeProvider _timeProvider; | ||
private readonly ILogger<LongRunningOperationService> _logger; | ||
|
||
private readonly TimeSpan _timeToWaitBetweenBackgroundTaskStatusChecks = TimeSpan.FromSeconds(10); |
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.
Perhaps we should have configuration for this, with these being the defaults if no configuration is provided?
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.
Also adjusted in 7c572e5.
Co-authored-by: Andy Butland <[email protected]>
e2a2bb6
to
f3c41e4
Compare
…ons-in-a-LB-friendly-way
Description
This pull request introduces a new
LongRunningOperationService
service that allows you to run/queue and track long running operations. Also added a repository to keep track of the status, and a few unit and integration tests.Adjusted the
ContentPublishingService.PublishBranchAsync()
andDatabaseCacheRebuilder.Rebuild()
to use the new service instead of usingIBackgroundTaskQueue
directly.Added a new background job
LongRunningOperationsCleanupJob
that deletes them after some time (now hardcoded to 1 day).Considerations:
Run()
parameterstype
andallowConcurrentExecution
.Methods:
Run()
- runs or schedules a task/operation.type
- type of operation, mostly relevant whenallowMultipleRunsOfType
is false, but also used to make sure that when an operation is queried the type matches.operation
- the task to run (Task
when no result,Task<T>
when a result is returned, which we need to store).runInBackground
- whether to queue the operation in the background, or run it and wait for the result before returning.allowConcurrentExecution
- whether you should be able to run or queue an operation when another of the same type is already running, example: database cache rebuild.GetStatus()
- gets the status of an operation by id.GetByType()
- gets a list of operations by type. Accepts a list of status to use as filter. Defaults to Enqueued or Running.GetResult()
- Get the result of an operation.TODOs
Check how to handle- will be done in a separate PRIndexingRebuilderService.TryRebuild