-
Notifications
You must be signed in to change notification settings - Fork 231
fix: Bulk export cleanup and notification occasionally not working on job expire #10366
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
fix: Bulk export cleanup and notification occasionally not working on job expire #10366
Conversation
pageBulkExportJobCronService, | ||
), | ||
const cleanUp = async (job: PageBulkExportJobDocument) => { | ||
await pageBulkExportJobCronService?.notifyExportResultAndCleanUp( |
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.
cleanUpAndDeleteBulkExportJobs ではなく notifyExportResultAndCleanUp を実行し、明示的に期限切れの通知を実行。
// The key is the id of a PageBulkExportJob, value is array of streams. | ||
private streamInExecutionMemo: { | ||
[key: string]: Readable; | ||
[key: string]: (Readable | Writable)[]; |
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.
readable と writable stream 両方をメモリ上に記録して置けるように変更。
// prevent overlapping cleanup | ||
if (!(err instanceof BulkExportJobStreamDestroyedByCleanupError)) { | ||
this.handleError(err, pageBulkExportJob); | ||
} |
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.
元々 cleanup で投げた BulkExportJobRestartedError, BulkExportJobExpiredError もここでキャッチし、handleError を実行することによって通知と、重複した cleanup 処理を実行していた。
今回 cleanup の destroy で生じたエラーは処理しないように変更。
(重複した cleanup を実行していたのは、destroy によって stream の処理が終わる前に cleanup が完了してしまう場合をカバーするためだった。これをカバーするために cleanup 関数側で stream close を待つように変更 1d807d0)
*/ | ||
async cleanUpExportJobResources( | ||
pageBulkExportJob: PageBulkExportJobDocument, | ||
restarted = false, |
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.
restarted かどうかは stream のエラーを handleError によって処理するときの場合分けに必要だったが、cleanup 時は handleError しなくなったため、不要となった。
What
bulk export job が expire した際、expire した旨の通知が届かないことがある事象を修正。
原因
実行を開始した readable stream をメモリ上に記録しておき、expire した際はそれを
BulkExportJobExpiredError
で destroy し、そのエラーを handleError で処理して notifyExportResultAndCleanUp に通知を任せる方針になっていた。これだと、readable stream の処理が軽い場合、次の expire した後の次の cleanup cron 実行までに完了していることがあり、通知がされない。
対応内容
そもそも通知が stream の存在に依存して行われていることがよくないため、
また、readable stream のみが cleanup されており、後段の writable stream が cleanup されていなかったため、writable stream もメモリ上に保管しておき、cleanup 対象とする。
task
https://redmine.weseek.co.jp/issues/172103