Skip to content

fix: shorten cache life#2631

Merged
jacopen merged 1 commit intomainfrom
avoid-cache-confrit
Sep 28, 2025
Merged

fix: shorten cache life#2631
jacopen merged 1 commit intomainfrom
avoid-cache-confrit

Conversation

@takaishi
Copy link
Contributor

@takaishi takaishi commented Sep 27, 2025

CloudFrontのキャッシュ有効期間を短くする。キャッシュポリシーは cloudnativedaysjp/terraform#234 で作成済み。手元では前日同時間に流したデータが翌日混ざることはなかったが、本当にこれで直るのかはあまり自信がない…

@takaishi takaishi marked this pull request as ready for review September 27, 2025 12:41
@takaishi takaishi self-assigned this Sep 27, 2025
@takaishi takaishi requested a review from a team September 27, 2025 12:41
@gitops-for-cloudnativedays gitops-for-cloudnativedays bot added the reviewapps Build ReviewApp environment automatically if this label is granted label Sep 27, 2025
@claude
Copy link

claude bot commented Sep 27, 2025

Claude finished @takaishi's task —— View job


📋 コードレビュー結果

このPRはCloudFrontのキャッシュ有効期間を短くするための変更です。主な変更点は以下の通りです:

🔍 変更内容の分析

  • 変更されたファイルを読んでコンテキストを理解する
  • コード品質とベストプラクティスを確認する
  • 潜在的なバグや問題をチェックする
  • パフォーマンスへの影響を評価する
  • セキュリティ上の懸念をチェックする
  • テストカバレッジを確認する
  • 建設的なフィードバックをまとめる

✅ 良い点

  1. キャッシュポリシーの細分化: 単一のキャッシュポリシーから、manifestsegment用の2つの異なるキャッシュポリシーに分離されています。これにより、コンテンツタイプに応じた適切なキャッシュ戦略を適用できます。

  2. パスパターンの改善:

    • *.m3u8 (マニフェストファイル)と一般的なセグメントファイルで異なるキャッシュ動作を設定
    • より細かな制御が可能になります
  3. 正確な数量計算: remove_origin_and_behaviorメソッドで、削除されるキャッシュ動作の数を正確に計算するように修正されています(109-111行目)

🔧 改善提案

  1. マジックナンバーの説明:

    # 177行目と208行目
    cache_policy_id: cache_policy('manifest').id
    cache_policy_id: cache_policy('segment').id

    'manifest''segment'という文字列がハードコードされていますが、これらを定数として定義することを検討してください。

  2. 重複コードの削減:
    新しいキャッシュ動作の定義(147-210行目)に重複が多く見られます。共通部分を抽出してメソッド化することを推奨します:

    def build_cache_behavior(path_pattern, cache_type)
      {
        path_pattern: path_pattern,
        target_origin_id: origin_endpoint_name.to_s,
        trusted_signers: { enabled: false, quantity: 0 },
        trusted_key_groups: { enabled: false, quantity: 0 },
        viewer_protocol_policy: 'https-only',
        # ... 他の共通設定 ...
        cache_policy_id: cache_policy(cache_type).id
      }
    end
  3. エラーハンドリングの改善:
    cache_policyメソッド(237行目)で、指定されたタイプのキャッシュポリシーが見つからない場合の処理がありません。nilを返す可能性があり、NoMethodErrorが発生する恐れがあります。

⚠️ 潜在的な問題

  1. キャッシュポリシーの存在確認:
    キャッシュポリシー(MediaPackageV2_manifest_prdなど)が事前に作成されていることが前提となっていますが、存在確認がありません。適切なエラーメッセージとともに例外を発生させることを推奨します。

  2. パフォーマンスの考慮:
    cache_policyメソッドで毎回CloudFrontのAPIを呼び出してポリシーのリストを取得していますが、これはインスタンス変数やクラス変数でキャッシュすることで改善できます。

🔒 セキュリティ

セキュリティ上の懸念は特に見つかりませんでした。AWS APIの呼び出しは適切に行われており、ハードコードされた認証情報も含まれていません。

🧪 テストカバレッジ

重要: このモデルに対する単体テストが見つかりませんでした。特に以下のケースのテストを追加することを強く推奨します:

  • 新しいcache_policyメソッドの動作(異なるタイプでの呼び出し)
  • add_origin_and_behaviorでの2つのキャッシュ動作の追加
  • キャッシュポリシーが見つからない場合のエラーハンドリング

📝 総評

このPRの目的であるキャッシュ有効期間の短縮は、マニフェストとセグメントで異なるキャッシュポリシーを使用することで実現されています。実装は機能的には問題ありませんが、コードの保守性とテストカバレッジの観点から改善の余地があります。特にテストの追加は必須と考えます。

gitops-for-cloudnativedays bot added a commit to cloudnativedaysjp/dreamkast-infra that referenced this pull request Sep 27, 2025
commit: cloudnativedaysjp/dreamkast@ee9855a
action URL: https://github.com/cloudnativedaysjp/dreamkast/actions/runs/18059923924

Co-authored-by: gitops-for-cloudnativedays[bot] <113280573+gitops-for-cloudnativedays[bot]@users.noreply.github.com>
@github-actions
Copy link

@github-actions
Copy link

Simplecov Report

Covered Threshold
64.37% 60%

@jacopen jacopen merged commit c84ce9d into main Sep 28, 2025
12 checks passed
@jacopen jacopen deleted the avoid-cache-confrit branch September 28, 2025 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewapps Build ReviewApp environment automatically if this label is granted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants