-
Notifications
You must be signed in to change notification settings - Fork 220
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
Feat/78039 135789 delete unnecessary page bulk export jobs #8974
Feat/78039 135789 delete unnecessary page bulk export jobs #8974
Conversation
|
…o feat/78039-135789-delete-unnecessary-page-bulk-export-jobs
…necessary-page-bulk-export-jobs
…o feat/78039-135789-delete-unnecessary-page-bulk-export-jobs
…o feat/78039-135789-delete-unnecessary-page-bulk-export-jobs
…o feat/78039-135789-delete-unnecessary-page-bulk-export-jobs
…necessary-page-bulk-export-jobs
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.
jest から vitest 利用への移行はとてもありがたい 👍
} | ||
// eslint-disable-next-line no-await-in-loop | ||
await this.cleanUpAndDeleteBulkExportJob(expiredExportJob); | ||
} |
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.
やむを得ない場合以外は no-await-in-loop は抑制するのではなく避けたい
- expiredExportJobs は膨大な量にはならないと思うので notifyExportResult を呼ぶのには Promise.allSettled 使っていいと思う
- cleanUp (削除)
- cleanUpExportJobResources 呼び出しは複数対応させるか Promise.allSettled 利用でいいんじゃないかな
- document データの削除はクエリ1発にしてほしい
- deleteExpiredExportJobs, deleteDownloadExpiredExportJobs, deleteFailedExportJobs で計3発打つのは許容
この対応は後続タスクでも可
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.
対応しました。
2674ad2
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.
補足)
export 中は PageBulkExportPageSnapshot が export 対象のページ数分 DB 上にあり、export が中断されると cleanUp 時にこれらが pageBulkExportService.cleanUpExportJobResources の実行により一括削除されます。
最初これの削除が並列で実行されるのは良く無いかなと思っていましたが、
- cronjob が 10 分おきに実行され、定期的に cleanUp されること
- limit parallel bulk export execution #9031 によって並列で実行される export job が制限されること (9031 では上限を 5 に設定しています)
を考えると、武井さんのいうように cronjob 側で Promise 数を制御する必要はないかなと思いました。
また、もし PageBulkExportPageSnapshot の一括削除の負荷の最大値を下げたい場合は、
PageBulkExportPageSnapshot.deleteMany({ pageBulkExportJob }), |
での deleteMany の実行を分割する方が得策かなと思いました。
…necessary-page-bulk-export-jobs
実装内容
task
https://redmine.weseek.co.jp/issues/135789
備考
PageBulkExportJobCronService のテストは https://redmine.weseek.co.jp/issues/150777 で実装予定