Skip to content

Conversation

@dengyh
Copy link
Collaborator

@dengyh dengyh commented Oct 30, 2025

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

代码审查总结

本次 PR 对任务清理逻辑进行了重要优化,引入了批处理和删除顺序控制。整体方向正确,但发现几个需要关注的问题:

🚨 严重问题

delete_records() 函数名称与实现不符

  • 函数注释声称"带重试机制",但实际只捕获异常并记录日志,没有实现任何重试逻辑
  • 建议:要么实现真正的重试机制(如 3 次重试),要么修改函数名和注释以反映实际行为

⚠️ 逻辑问题

批次失败后继续处理可能导致数据不一致

  • 第 192 行:当某批次失败时,代码 continue 继续处理下一批
  • 问题:pre_delete_pipeline_instance_data 信号可能已经修改了部分数据,但删除失败会导致不一致
  • 建议:考虑是否应该在批次失败时停止整个任务,或者将信号发送移到事务内部

硬编码的批次大小

  • 第 176 行:task_batch_size = 20 应该作为配置项,便于根据实际情况调整

⚡ 性能考虑

批次间休眠时间可能不足

  • 第 197 行:time.sleep(0.5) 对于 20 个任务的批次可能太短
  • 建议:考虑增加到 1-2 秒,或根据批次大小动态调整

✨ 优点

  • 删除顺序设计合理,按依赖关系从叶子到根删除
  • 批处理策略有效降低单次事务压力
  • 日志记录详细,便于问题排查

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

代码审查

审查了本次 PR 的任务清理优化重构,发现以下问题:

🚨 严重问题

1. delete_records() 函数名称与实现不符 (第 37-48 行)

  • 函数注释声称"带重试机制的删除操作",但实际只捕获异常并记录日志,没有实现任何重试逻辑
  • 建议:要么实现真正的重试机制(如使用 tenacity 库或手动重试 3 次),要么修改函数名和注释以反映实际行为

⚠️ 逻辑问题

2. 批次失败后继续处理可能导致数据不一致 (第 192-194 行)

  • 当某批次删除失败时,代码使用 continue 继续处理下一批
  • 问题:pre_delete_pipeline_instance_data 信号(第 217 行)可能已经修改了部分数据,但如果后续删除失败会导致数据不一致
  • 建议:考虑是否应该在批次失败时中止整个任务,或者将信号发送移到事务提交之后

3. 硬编码的批次大小 (第 176 行)

  • task_batch_size = 20 应该作为配置项(如 settings.CLEAN_EXPIRED_V2_TASK_INNER_BATCH_SIZE
  • 便于根据不同环境的数据库负载动态调整

⚡ 性能考虑

4. 批次间休眠时间可能不足 (第 197 行)

  • time.sleep(0.5) 对于包含 20 个任务(可能涉及数千条记录)的批次可能太短
  • 建议:增加到 1-2 秒,或根据批次大小动态调整(如 task_batch_size * 0.05

✨ 优点

  • ✅ 删除顺序设计合理,按依赖关系从叶子到根删除,有效避免外键冲突
  • ✅ 批处理策略有效降低单次事务的锁定时间
  • ✅ 日志记录详细完整,便于问题排查
  • ✅ 异常处理不会导致整个清理任务崩溃

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 1.96078% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.63%. Comparing base (396ee46) to head (fa920c2).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
gcloud/contrib/cleaner/tasks.py 0.00% 50 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8085      +/-   ##
==========================================
- Coverage   58.68%   58.63%   -0.06%     
==========================================
  Files         664      664              
  Lines       35514    35546      +32     
==========================================
  Hits        20842    20842              
- Misses      14672    14704      +32     
Files with missing lines Coverage Δ
gcloud/analysis_statistics/tasks.py 16.93% <100.00%> (ø)
gcloud/contrib/cleaner/tasks.py 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 396ee46...fa920c2. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dengyh dengyh merged commit bc0ca67 into TencentBlueKing:master Oct 30, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants