From c1cfe0db301acb3661cb9d432f08804b195a12ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 22 Mar 2022 16:21:37 +0100 Subject: [PATCH] review updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/shiny-seas-yell.md | 15 +++- .../src/tasks/PluginTaskSchedulerImpl.ts | 8 +-- plugins/tech-insights-backend/README.md | 1 + plugins/tech-insights-backend/api-report.md | 8 +-- .../service/fact/FactRetrieverEngine.test.ts | 2 +- .../src/service/fact/FactRetrieverEngine.ts | 70 ++++++++++--------- .../src/service/fact/createFactRetriever.ts | 9 ++- .../src/service/techInsightsContextBuilder.ts | 2 +- 8 files changed, 65 insertions(+), 50 deletions(-) diff --git a/.changeset/shiny-seas-yell.md b/.changeset/shiny-seas-yell.md index d1dd2dcb29..9a939701d8 100644 --- a/.changeset/shiny-seas-yell.md +++ b/.changeset/shiny-seas-yell.md @@ -2,4 +2,17 @@ '@backstage/plugin-tech-insights-backend': minor --- -Updates tech-insights to use backend-tasks as the Fact Retriever scheduler. +This backend now uses the `@backstage/backend-tasks` package facilities for scheduling fact retrievers. + +**BREAKING**: The `buildTechInsightsContext` function now takes an additional field in its options argument: `scheduler`. This is an instance of `PluginTaskScheduler`, which can be found in your backend initialization code's `env`. + +```diff + const builder = buildTechInsightsContext({ + logger: env.logger, + config: env.config, + database: env.database, + discovery: env.discovery, ++ scheduler: env.scheduler, + factRetrievers: [ /* ... */ ], + }); +``` diff --git a/packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts b/packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts index 7a206712be..44f46e7237 100644 --- a/packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts +++ b/packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts @@ -41,11 +41,9 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler { // check if task exists const rows = await knex(DB_TASKS_TABLE) - .count({ count: '*' }) + .select(knex.raw(1)) .where('id', '=', id); - - // validate the task exists - if (rows[0].count !== 1) { + if (rows.length !== 1) { throw new NotFoundError(`Task ${id} does not exist`); } @@ -56,7 +54,7 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler { next_run_start_at: knex.fn.now(), }); if (updatedRows < 1) { - throw new ConflictError(`task ${id} is currently running`); + throw new ConflictError(`Task ${id} is currently running`); } } diff --git a/plugins/tech-insights-backend/README.md b/plugins/tech-insights-backend/README.md index 60c3477efe..55653e097b 100644 --- a/plugins/tech-insights-backend/README.md +++ b/plugins/tech-insights-backend/README.md @@ -35,6 +35,7 @@ export default async function createPlugin( config: env.config, database: env.database, discovery: env.discovery, + scheduler: env.scheduler, factRetrievers: [], // Fact retrievers registrations you want tech insights to use }); diff --git a/plugins/tech-insights-backend/api-report.md b/plugins/tech-insights-backend/api-report.md index 08c6e82385..210aa50af9 100644 --- a/plugins/tech-insights-backend/api-report.md +++ b/plugins/tech-insights-backend/api-report.md @@ -27,11 +27,9 @@ export const buildTechInsightsContext: < ) => Promise>; // @public -export function createFactRetrieverRegistration({ - cadence, - factRetriever, - lifecycle, -}: FactRetrieverRegistrationOptions): FactRetrieverRegistration; +export function createFactRetrieverRegistration( + options: FactRetrieverRegistrationOptions, +): FactRetrieverRegistration; // @public export function createRouter< diff --git a/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.test.ts b/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.test.ts index a019c533a1..1881606ede 100644 --- a/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.test.ts +++ b/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.test.ts @@ -194,7 +194,7 @@ describe('FactRetrieverEngine', () => { } engine = await createEngine(databaseId, insertCallback, () => {}); - engine.schedule(); + await engine.schedule(); const job: FactRetrieverRegistration = engine.getJobRegistration( testFactRetriever.id, ); diff --git a/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.ts b/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.ts index 9f9c91b4ee..25e7610809 100644 --- a/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.ts +++ b/plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.ts @@ -39,7 +39,7 @@ function duration(startTimestamp: [number, number]): string { } export class FactRetrieverEngine { - constructor( + private constructor( private readonly repository: TechInsightsStore, private readonly factRetrieverRegistry: FactRetrieverRegistry, private readonly factRetrieverContext: FactRetrieverContext, @@ -49,14 +49,7 @@ export class FactRetrieverEngine { private readonly defaultTimeout?: Duration, ) {} - static async create({ - repository, - factRetrieverRegistry, - factRetrieverContext, - scheduler, - defaultCadence, - defaultTimeout, - }: { + static async create(options: { repository: TechInsightsStore; factRetrieverRegistry: FactRetrieverRegistry; factRetrieverContext: FactRetrieverContext; @@ -64,6 +57,15 @@ export class FactRetrieverEngine { defaultCadence?: string; defaultTimeout?: Duration; }) { + const { + repository, + factRetrieverRegistry, + factRetrieverContext, + scheduler, + defaultCadence, + defaultTimeout, + } = options; + await Promise.all( factRetrieverRegistry .listRetrievers() @@ -81,31 +83,35 @@ export class FactRetrieverEngine { ); } - schedule() { + async schedule() { const registrations = this.factRetrieverRegistry.listRegistrations(); const newRegs: string[] = []; - registrations.forEach(async registration => { - const { factRetriever, cadence, lifecycle, timeout } = registration; - const cronExpression = - cadence || this.defaultCadence || randomDailyCron(); - const timeLimit = - timeout || this.defaultTimeout || Duration.fromObject({ minutes: 5 }); - try { - await this.scheduler.scheduleTask({ - id: factRetriever.id, - frequency: { cron: cronExpression }, - fn: this.createFactRetrieverHandler(factRetriever, lifecycle), - timeout: timeLimit, - }); - } catch (e) { - throw new Error( - `Failed to schedule fact retriever ${factRetriever.id}, ${e}`, - ); - } - newRegs.push(factRetriever.id); - }); + + await Promise.all( + registrations.map(async registration => { + const { factRetriever, cadence, lifecycle, timeout } = registration; + const cronExpression = + cadence || this.defaultCadence || randomDailyCron(); + const timeLimit = + timeout || this.defaultTimeout || Duration.fromObject({ minutes: 5 }); + try { + await this.scheduler.scheduleTask({ + id: factRetriever.id, + frequency: { cron: cronExpression }, + fn: this.createFactRetrieverHandler(factRetriever, lifecycle), + timeout: timeLimit, + }); + newRegs.push(factRetriever.id); + } catch (e) { + this.logger.warn( + `Failed to schedule fact retriever ${factRetriever.id}, ${e}`, + ); + } + }), + ); + this.logger.info( - `Scheduled ${newRegs.length} fact retrievers to Fact Retriever Engine through Backend Tasks`, + `Scheduled ${newRegs.length}/${registrations.length} fact retrievers into the tech-insights engine`, ); } @@ -114,7 +120,7 @@ export class FactRetrieverEngine { } async triggerJob(ref: string): Promise { - return this.scheduler.triggerTask(ref); + await this.scheduler.triggerTask(ref); } private createFactRetrieverHandler( diff --git a/plugins/tech-insights-backend/src/service/fact/createFactRetriever.ts b/plugins/tech-insights-backend/src/service/fact/createFactRetriever.ts index bd85b3c873..de49888e1b 100644 --- a/plugins/tech-insights-backend/src/service/fact/createFactRetriever.ts +++ b/plugins/tech-insights-backend/src/service/fact/createFactRetriever.ts @@ -61,11 +61,10 @@ export type FactRetrieverRegistrationOptions = { * \{ maxItems: 7 \} -- This fact retriever will leave 7 newest items in the database when it is run * */ -export function createFactRetrieverRegistration({ - cadence, - factRetriever, - lifecycle, -}: FactRetrieverRegistrationOptions): FactRetrieverRegistration { +export function createFactRetrieverRegistration( + options: FactRetrieverRegistrationOptions, +): FactRetrieverRegistration { + const { cadence, factRetriever, lifecycle } = options; return { cadence, factRetriever, diff --git a/plugins/tech-insights-backend/src/service/techInsightsContextBuilder.ts b/plugins/tech-insights-backend/src/service/techInsightsContextBuilder.ts index 5e599ab664..4f8f2ed928 100644 --- a/plugins/tech-insights-backend/src/service/techInsightsContextBuilder.ts +++ b/plugins/tech-insights-backend/src/service/techInsightsContextBuilder.ts @@ -124,7 +124,7 @@ export const buildTechInsightsContext = async < }, }); - factRetrieverEngine.schedule(); + await factRetrieverEngine.schedule(); if (factCheckerFactory) { const factChecker = factCheckerFactory.construct(