Skip to content
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

Conversation

arafubeatbox
Copy link
Contributor

@arafubeatbox arafubeatbox commented Jul 19, 2024

実装内容

  • 不要になった PageBulkExportJob とその周りのリソースのお掃除をする cron を実装
    • エクスポート処理の期限を過ぎたジョブ: リソースの削除、ユーザにその旨を通知
    • エクスポートが失敗したジョブ: 万が一失敗した時点で削除できなかったリソースがある場合はそれらを削除し、ジョブも削除
    • エクスポートに成功し、ダウンロード期限が過ぎているジョブ: attachment を削除し、万が一成功した時点で削除できなかったリソースがある場合はそれらも削除し、ジョブも削除
  • cron service のリファクタリング
    • CronService という abstract class を実装し、QuestionnaireCronService と今回実装した PageBulkExportJobCronService でそれを継承
    • QuestionnaireCronService は crowi 変数を使う必要がなくなったので、テストを jest から vitest に移行

task

https://redmine.weseek.co.jp/issues/135789

備考

PageBulkExportJobCronService のテストは https://redmine.weseek.co.jp/issues/150777 で実装予定

Copy link

changeset-bot bot commented Jul 19, 2024

⚠️ No Changeset found

Latest commit: 946e158

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

…o feat/78039-135789-delete-unnecessary-page-bulk-export-jobs
@arafubeatbox arafubeatbox changed the base branch from feat/page-bulk-export to feat/78040-135788-resume-suspended-bulk-export-job July 19, 2024 08:33
Base automatically changed from feat/78040-135788-resume-suspended-bulk-export-job to feat/page-bulk-export August 13, 2024 07:54
@arafubeatbox arafubeatbox marked this pull request as ready for review August 13, 2024 15:36
apps/app/src/server/crowi/index.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jest から vitest 利用への移行はとてもありがたい 👍

apps/app/src/server/service/config-loader.ts Outdated Show resolved Hide resolved
}
// eslint-disable-next-line no-await-in-loop
await this.cleanUpAndDeleteBulkExportJob(expiredExportJob);
}
Copy link
Member

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発打つのは許容

この対応は後続タスクでも可

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

対応しました。
2674ad2

Copy link
Contributor Author

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 の実行を分割する方が得策かなと思いました。

@yuki-takei yuki-takei merged commit 6201af7 into feat/page-bulk-export Aug 26, 2024
2 of 5 checks passed
@yuki-takei yuki-takei deleted the feat/78039-135789-delete-unnecessary-page-bulk-export-jobs branch August 26, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants