Skip to content

Conversation

@littleniannian
Copy link
Collaborator

@littleniannian littleniannian commented Mar 20, 2025

User description

关联的 issue

https://github.com/actiontech/sqle-ee/issues/2280

描述你的变更

  • 定义分布式锁采集的Model

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc


Description

  • 新增分布式锁数据模型与处理逻辑

  • 更新本地化信息文件添加分布式锁相关术语

  • 修改审计计划任务以支持分布式锁

  • 优化数据锁相关的数据库操作


Changes walkthrough 📝

Relevant files
Documentation
3 files
message_zh.go
添加分布式锁相关本地化信息                                                                                       
+44/-27 
active.en.toml
添加Distributed Lock相关英文本地化                                                               
+17/-0   
active.zh.toml
添加分布式锁相关中文本地化                                                                                       
+17/-0   
Enhancement
5 files
instance_audit_plan.go
引入DataLock模型及相关数据库操作                                                                         
+98/-0   
utils.go
更新自动迁移列表以包含DataLock模型                                                                       
+1/-0     
meta.go
添加分布式锁类型至审计计划类型                                                                                   
+13/-12 
metrics.go
增加锁相关指标名称                                                                                               
+27/-0   
task_wrap.go
处理分布式锁的SQL收集任务                                                                                     
+51/-10 

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @actiontech-bot actiontech-bot requested a review from iwanghc March 20, 2025 02:14
    @github-actions
    Copy link

    github-actions bot commented Mar 20, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit c999c48)

    🎫 Ticket compliance analysis ✅

    2280 - Fully compliant

    Compliant requirements:

    • fix:remove replace label
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @github-actions
    Copy link

    github-actions bot commented Mar 20, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 9eee7e6

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    正确处理错误包装

    PushSQLToDataLock函数中,errors.New函数不接受错误类型作为参数。应使用fmt.Errorf或其他错误包装方法以保留原始错误信息。

    sqle/model/instance_audit_plan.go [298]

    -return errors.New(errors.ConnectStorageError, err)
    +return fmt.Errorf("%w: %v", errors.ConnectStorageError, err)
    Suggestion importance[1-10]: 8

    __

    Why: 使用 fmt.Errorf 进行错误包装可以保留原始错误信息,这对于准确的错误处理和调试至关重要。

    Medium
    添加删除条件以防误删


    在执行删除操作时缺少WHERE条件,会删除所有data_locks记录。应根据audit_plan_idinstance_audit_plan_id添加过滤条件以防止误删。

    sqle/model/instance_audit_plan.go [287-289]

    -if err := tx.Exec("DELETE FROM data_locks").Error; err != nil {
    +if err := tx.Where("audit_plan_id = ? AND instance_audit_plan_id = ?", auditPlanId, instanceAuditPlanId).Delete(&DataLock{}).Error; err != nil {
       return err
     }
    Suggestion importance[1-10]: 6

    __

    Why: 添加 WHERE 条件可以防止误删除 data_locks 表中的所有记录,从而增强数据操作的稳健性和安全性。

    Low

    Previous suggestions

    Suggestions up to commit 591789f
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    修正事务中数据库操作

    在事务中执行删除操作时,应使用事务对象 tx 来确保操作属于同一事务,避免数据不一致。

    sqle/model/instance_audit_plan.go [287]

    -err := s.db.Exec("DELETE FROM data_locks").Error
    +err := tx.Exec("DELETE FROM data_locks").Error
    Suggestion importance[1-10]: 8

    __

    Why: 通过使用事务对象 tx 执行删除操作,可以确保该操作属于同一事务,从而避免数据不一致的问题。这对于维护数据完整性非常重要。

    Medium
    修正错误处理逻辑

    无论 err 是否为 nil,当前代码都会返回一个新的错误对象,这可能导致在没有错误时仍然返回错误。应在 err 不为 nil 时才返回错误。

    sqle/model/instance_audit_plan.go [298]

    -return errors.New(errors.ConnectStorageError, err)
    +if err != nil {
    +    return errors.New(errors.ConnectStorageError, err)
    +}
    +return nil
    Suggestion importance[1-10]: 8

    __

    Why: 现有代码在没有错误时仍然返回一个新的错误对象,这可能导致错误处理不当。通过仅在 err 不为 nil 时返回错误,可以确保错误处理的准确性。

    Medium
    Suggestions up to commit 591789f
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    修正事务内DELETE操作

    在事务中使用tx.Exec替代s.db.Exec,确保删除操作包含在事务内,以防止数据不一致。

    sqle/model/instance_audit_plan.go [287]

    -if err := s.db.Exec("DELETE FROM data_locks").Error; err != nil {
    +if err := tx.Exec("DELETE FROM data_locks").Error; err != nil {
    Suggestion importance[1-10]: 9

    __

    Why: 确保删除操作包含在事务内,防止数据不一致,关键的事务性修正。

    High
    增加对sql.Info的空值检查

    在访问sql.Info之前,添加非空检查以防止nil指针解引用,从而避免程序崩溃。

    sqle/server/auditplan/task_wrap.go [340-341]

    +if sql.Info == nil {
    +    log.Logger().Error("sql.Info is nil")
    +    continue
    +}
     GrantedLockId:           sql.Info.Get(MetricNameGrantedLockId).String(),
     WaitingLockId:           sql.Info.Get(MetricNameWaitingLockId).String(),
    Suggestion importance[1-10]: 7

    __

    Why: 增加null检查可以防止潜在的nil指针错误,提升代码稳定性,具有中等重要性。

    Medium
    Suggestions up to commit fe23797
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    使用事务确保数据操作原子性

    为确保数据操作的原子性,建议将删除和创建操作封装在事务中,以防止在删除后创建失败导致数据丢失。

    sqle/model/instance_audit_plan.go [287-293]

    -if err := s.db.Exec("DELETE FROM data_locks").Error; err != nil {
    +// 使用事务封装删除和创建操作
    +tx := s.db.Begin()
    +if err := tx.Exec("DELETE FROM data_locks").Error; err != nil {
    +	tx.Rollback()
     	return err
     }
    -if len(dataLocks) == 0 {
    -	return nil
    +if len(dataLocks) > 0 {
    +	if err := tx.Create(dataLocks).Error; err != nil {
    +		tx.Rollback()
    +		return err
    +	}
     }
    -return s.db.Create(dataLocks).Error
    +return tx.Commit().Error
    Suggestion importance[1-10]: 9

    __

    Why: 该建议通过事务封装删除和创建操作,确保数据操作的原子性,防止在删除后创建失败导致数据丢失,显著提高了代码的可靠性。

    High
    Suggestions up to commit 74152c0
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    检查删除操作的错误返回

    在执行删除操作后,应检查Exec方法的错误返回值,以确保删除成功,防止潜在的数据不一致或资源泄漏。

    sqle/model/instance_audit_plan.go [287]

    -s.db.Exec("DELETE FROM data_locks")
    +// 新增前删除,保证数据的实时性
    +if err := s.db.Exec("DELETE FROM data_locks").Error; err != nil {
    +    return err
    +}
    Suggestion importance[1-10]: 8

    __

    Why: 该建议通过检查删除操作的错误返回,确保删除成功,提升代码的可靠性并防止潜在的数据不一致或资源泄漏问题。

    Medium

    # Conflicts:
    #	sqle/locale/message_zh.go
    @github-actions
    Copy link

    Persistent review updated to latest commit 74152c0

    @github-actions
    Copy link

    Persistent review updated to latest commit fe23797

    @github-actions
    Copy link

    Persistent review updated to latest commit 591789f

    1 similar comment
    @github-actions
    Copy link

    Persistent review updated to latest commit 591789f

    @github-actions
    Copy link

    Persistent review updated to latest commit 9eee7e6

    - 通用化 SelectDistinct 函数,支持查询不同列
    - 优化数据锁删除逻辑,仅删除特定审计计划相关的记录
    - 调整 pushSQLToDataLock 函数签名,增加审计计划参数
    - 优化 TaskWrapper 中的相关函数调用
    @github-actions
    Copy link

    Persistent review updated to latest commit c999c48

    @github-actions
    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @LordofAvernus LordofAvernus merged commit 8ccd9cf into main Mar 21, 2025
    4 checks passed
    @littleniannian littleniannian deleted the feat_tdsql_lock_data branch March 21, 2025 09:46
    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.

    3 participants