-
Notifications
You must be signed in to change notification settings - Fork 7
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
[TTAHUB-3097] Implement transactions for worker processes #2233
Conversation
@thewatermethod @nvms @AdamAdHocTeam @kcirtapfromspace @kryswisnaskas @hardwarehuman |
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.
Let's simplify transactionWrapper
a little bit:
export default function transactionWrapper(originalFunction, context = '') {
return async function wrapper(req, res, next) {
const startTime = Date.now();
try {
// eslint-disable-next-line @typescript-eslint/return-await
return await sequelize.transaction(async () => {
try {
await addAuditTransactionSettings(sequelize, null, null, 'transaction', originalFunction.name);
const result = await originalFunction(req, res, next);
const duration = Date.now() - startTime;
auditLogger.info(`${originalFunction.name} ${context} execution time: ${duration}ms`);
removeFromAuditedTransactions();
return result;
} catch (err) {
auditLogger.error(`Error executing ${originalFunction.name} ${context}: ${err.message}`);
throw err;
}
});
} catch (err) {
return handleErrors(req, res, err, logContext);
}
};
}
So we don't have to do that error = err
thing, which isn't needed. Also the result can be returned inside the try, so it doesn't need to be in the upper scope. Just a bit easier to read IMO.
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.
All functionality looks to still be there/work 👍
I'd grab Jon's changes to the sequelizeConnectionError stuff, or wait until that's merged into main, so we don't merge anything that throws errors in the error handler |
|
||
try { | ||
const responseBody = typeof error === 'object' && error !== null | ||
? { ...error, errorStack: error?.stack } |
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.
Wouldn't ...error also contain error.stack. Why do we need to put it in the property errorStack?
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.
just matching what the original code had
let operation; | ||
let label; | ||
|
||
if (error instanceof Sequelize.Error) { |
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.
why do we check instanceof Sequelize.Error here but above we do operation !== 'SequelizeError'.
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.
just matching original logic
* @param {Object} logContext - The context for logging. | ||
*/ | ||
export const handleUnexpectedWorkerError = (job, error, logContext) => { | ||
logger.error(`${logContext.namespace} - Unexpected error in catch block - ${error}`); |
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.
Will call stack be visible here?
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 think so
@@ -35,7 +40,8 @@ const processScanQueue = () => { | |||
scanQueue.on('failed', onFailedScanQueue); | |||
scanQueue.on('completed', onCompletedScanQueue); | |||
increaseListeners(scanQueue); | |||
scanQueue.process((job) => processFile(job.data.key)); | |||
const process = (job) => processFile(job.data.key); | |||
scanQueue.process(transactionQueueWrapper(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.
Do we need to pass a second param here to the transactionQueueWrapper like in S3 and Resource queue's?
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 scan queue never passed a key, so there is no associated value to use as a context
Revert "Merge pull request #2233 from HHS/TTAHUB-3097/worker-transact…
…transactions"" This reverts commit f749515.
Description of change
This update introduces transaction handling for worker processes in the Head Start TTA Data Platform. The main changes include:
How to test
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
After merge/deploy