-
Notifications
You must be signed in to change notification settings - Fork 14
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
#4076 - Queue Monitoring - Schedulers Refactor (SFAS Integration) #4097
#4076 - Queue Monitoring - Schedulers Refactor (SFAS Integration) #4097
Conversation
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.
...rocessors/schedulers/sfas-integration/_tests_/sims-to-sfas-integration.scheduler.e2e-spec.ts
Show resolved
Hide resolved
* @param job job. | ||
* @returns process summary. | ||
* @param _job process job. | ||
* @param processSummary process summary for logging. | ||
*/ | ||
@Process() |
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 decorator is expected to be removed right?
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 good, nice refactor of scheduler.
// Execute the import of all files records. | ||
await processInParallel( | ||
(record) => | ||
this.importRecord(record, downloadResult.header.creationDate, result), | ||
(record) => this.importRecord(record, downloadResult.header.creationDate), |
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.
Can we pass the processSummary to the importRecord
method? So that the errors logged inside that method will have both job and console logs.
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.
By doing further review I understood that when error is thrown the parent method will take care of the cross-cutting concerns so you can ignore the comment.
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.
I did not believe it was necessary since the first error should interrupt the import and it will logged either way.
const mockedJob = mockBullJob<void>(); | ||
|
||
// Act/Assert | ||
await expect(processor.processQueue(mockedJob.job)).rejects.toThrow( |
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.
👍
Good job @andrewsignori-aot. Just one comment on the decorator. |
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.
Great start. Looks good. Please remove the un-used import here sims-to-sfas-integration.scheduler.ts
Quality Gate passedIssues Measures |
SFASProcessingResult
with theprocessSummary
. This refactor is not expected to be done for every scheduler.