-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add SeederApi PlayData delete scheduled job #7281
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?
Changes from 4 commits
ccec83e
036268a
bf73bfa
ee5cce8
0c10e4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| using Bit.Core; | ||
| using Bit.Core.Jobs; | ||
| using Bit.Core.Settings; | ||
| using Quartz; | ||
|
|
||
| namespace Bit.SeederApi.Jobs; | ||
|
|
||
| public class AliveJob : BaseJob | ||
| { | ||
| private readonly GlobalSettings _globalSettings; | ||
| private HttpClient _httpClient = new HttpClient(); | ||
|
|
||
| public AliveJob( | ||
| GlobalSettings globalSettings, | ||
| ILogger<AliveJob> logger) | ||
| : base(logger) | ||
| { | ||
| _globalSettings = globalSettings; | ||
| } | ||
|
|
||
| protected async override Task ExecuteJobAsync(IJobExecutionContext context) | ||
| { | ||
| _logger.LogInformation(Constants.BypassFiltersEventId, "Execute job task: Keep alive"); | ||
| var response = await _httpClient.GetAsync(_globalSettings.BaseServiceUri.Admin); | ||
| _logger.LogInformation(Constants.BypassFiltersEventId, "Finished job task: Keep alive, {StatusCode}", | ||
| response.StatusCode); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // FIXME: Update this file to be null safe and then delete the line below | ||
theMickster marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| #nullable disable | ||
|
|
||
| using Bit.Core; | ||
| using Bit.Core.Jobs; | ||
| using Bit.SeederApi.Commands.Interfaces; | ||
| using Bit.SeederApi.Queries.Interfaces; | ||
| using Quartz; | ||
|
|
||
| namespace Bit.SeederApi.Jobs; | ||
|
|
||
| public class DeleteOldPlayDataJob : BaseJob | ||
| { | ||
| private readonly IGetAllPlayIdsQuery _getAllPlayIdsQuery; | ||
| private readonly IDestroyBatchScenesCommand _destroyBatchScenesCommand; | ||
|
|
||
| public DeleteOldPlayDataJob( | ||
| IGetAllPlayIdsQuery getAllPlayIdsQuery, | ||
| IDestroyBatchScenesCommand destroyBatchScenesCommand, | ||
| ILogger<DeleteOldPlayDataJob> logger) | ||
| : base(logger) | ||
| { | ||
| _getAllPlayIdsQuery = getAllPlayIdsQuery; | ||
| _destroyBatchScenesCommand = destroyBatchScenesCommand; | ||
| } | ||
|
|
||
| protected async override Task ExecuteJobAsync(IJobExecutionContext context) | ||
| { | ||
| _logger.LogInformation(Constants.BypassFiltersEventId, "Execute job task: DeleteOldPlayDataJob"); | ||
| var olderThan = DateTime.UtcNow.AddDays(-1); | ||
| var playIds = _getAllPlayIdsQuery.GetAllPlayIds(olderThan); | ||
| await _destroyBatchScenesCommand.DestroyAsync(playIds); | ||
| _logger.LogInformation(Constants.BypassFiltersEventId, "Finished job task: DeleteOldPlayDataJob. Deleted {PlayIdCount} root items", playIds.Count); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| using Bit.Core.Jobs; | ||
| using Bit.Core.Settings; | ||
| using Quartz; | ||
|
|
||
| namespace Bit.SeederApi.Jobs; | ||
|
|
||
| public class JobsHostedService : BaseJobsHostedService | ||
| { | ||
| public JobsHostedService( | ||
| GlobalSettings globalSettings, | ||
| IServiceProvider serviceProvider, | ||
| ILogger<JobsHostedService> logger, | ||
| ILogger<JobListener> listenerLogger) | ||
| : base(globalSettings, serviceProvider, logger, listenerLogger) { } | ||
|
|
||
| public override async Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| var everyTopOfTheHourTrigger = TriggerBuilder.Create() | ||
| .WithIdentity("EveryTopOfTheHourTrigger") | ||
| .StartNow() | ||
| .WithCronSchedule("0 0 * * * ?") | ||
| .Build(); | ||
| var everyFifteenMinutesTrigger = TriggerBuilder.Create() | ||
| .WithIdentity("everyFifteenMinutesTrigger") | ||
| .StartNow() | ||
| .WithCronSchedule("0 */15 * ? * *") | ||
| .Build(); | ||
|
|
||
|
|
||
| var jobs = new List<Tuple<Type, ITrigger>> | ||
| { | ||
| new Tuple<Type, ITrigger>(typeof(AliveJob), everyTopOfTheHourTrigger), | ||
| new Tuple<Type, ITrigger>(typeof(DeleteOldPlayDataJob), everyFifteenMinutesTrigger), | ||
| }; | ||
|
|
||
| Jobs = jobs; | ||
| await base.StartAsync(cancellationToken); | ||
| } | ||
|
|
||
| public static void AddJobsServices(IServiceCollection services) | ||
| { | ||
| services.AddTransient<AliveJob>(); | ||
| services.AddTransient<DeleteOldPlayDataJob>(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,9 @@ public void ConfigureServices(IServiceCollection services) | |
| services.AddQueries(); | ||
|
|
||
| services.AddControllers(); | ||
|
|
||
| Jobs.JobsHostedService.AddJobsServices(services); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 Deleting data with a hosted service always give me a little heartburn. Have we implemented opt-in techniques using environment settings in other places, by chance? I have done that in the past where my teams implemented background services that mutated data and gave us a better feel of control (especially when developing) knowing that one had to explicitly set the setting to ON to mutate/delete data?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Are you concerned with unintentional data loss? To some extent the goal of this job is to enforce that play data is ephemeral. That can be a server setting if we want, I suppose, but maybe a better tweak would be a lifetime?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the ephemeral environments, I am not too concerned because I would not expect an ephemeral environment to live long enough to see the job run. Since the intention is really deleting this ephemeral data from long lived environments then I'm cool with it. Only real issue is if you're developing tests locally or on one of the QA team VMs and you're expecting data to be there over a couple days (like a weekend) then you'd better be ready for the Seeder to clean itself up. Did we add a README.md? (Sorry on the GH app so hard to go back n forth). |
||
| services.AddHostedService<Jobs.JobsHostedService>(); | ||
| } | ||
|
|
||
| public void Configure( | ||
|
|
||
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.
🤔 Noting this as a known anti-pattern that causes socket exhaustion. Did we copy this from the src/Admin job?
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.
sure did. I don't know what issue, do we have a fix? Is this bump job unnecessary?
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.
The AliveJob is fine — the issue is
private HttpClient _httpClient = new HttpClient(). Directly instantiating HttpClient as a field is a known antipattern that causes socket exhaustion under load because each instance gets its own connection pool and doesn't respect DNS TTLs.The fix is to inject
IHttpClientFactoryand callhttpClientFactory.CreateClient()in the constructor. We'll also needservices.AddHttpClient()in program startup. The job itself can stay as-is.