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

fix can restart cn when tn crash #17115

Merged
merged 4 commits into from
Jun 25, 2024
Merged

Conversation

iamlinjunhong
Copy link
Contributor

@iamlinjunhong iamlinjunhong commented Jun 24, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/3516 #https://github.com/matrixorigin/MO-Cloud/issues/3537

What this PR does / why we need it:

fix can restart cn when tn crash


PR Type

Bug fix, Tests


Description

  • Enhanced logging for service status changes and errors in lock_table_allocator.go.
  • Added error logging for abnormal lock service status in lock_table_keeper.go.
  • Introduced new logging functions and enhanced existing ones in log.go.
  • Improved logging and status setting in service.go.
  • Added new tests to cover service restart and lock status issues in service_test.go.

Changes walkthrough 📝

Relevant files
Enhancement
lock_table_allocator.go
Enhance logging and status handling in lock table allocator

pkg/lockservice/lock_table_allocator.go

  • Added logging for service status changes.
  • Enhanced error logging with service status details.
  • Introduced getStatus method to serviceBinds.
  • Improved handling of abnormal lock service status.
  • +25/-6   
    lock_table_keeper.go
    Add error logging for lock service status in keeper           

    pkg/lockservice/lock_table_keeper.go

    • Added error logging for abnormal lock service status.
    +7/-0     
    log.go
    Introduce and enhance logging functions for service status

    pkg/lockservice/log.go

  • Added new logging functions for service status and status changes.
  • Enhanced existing logging functions with additional parameters.
  • +17/-2   
    service.go
    Improve logging and status setting in service                       

    pkg/lockservice/service.go

  • Added service ID to logCanLockOnService call.
  • Adjusted order of status logging and setting in setStatus.
  • +2/-2     
    Tests
    service_test.go
    Add tests for service restart and lock status issues         

    pkg/lockservice/service_test.go

  • Added new tests for issues related to service restart and lock status.

  • +187/-0 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The new logging functions such as logServiceStatus and logStatusChange are added to improve logging. However, it's important to ensure that these functions do not introduce any performance overhead due to excessive logging, especially in high-throughput environments.
    Code Clarity:
    The changes in lockTableAllocator.go introduce multiple status checks and logging within critical sections of code handling transaction and service statuses. It's crucial to review these changes for potential race conditions or deadlocks.

    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 24, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a check to ensure the new status is different from the current status before logging and updating it

    Add a check to ensure the new status is different from the current status before logging
    and updating it to avoid unnecessary operations.

    pkg/lockservice/service.go [389-391]

    -logStatusChange(s.mu.status, status)
    -s.mu.status = status
    +if s.mu.status != status {
    +    logStatusChange(s.mu.status, status)
    +    s.mu.status = status
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a condition to check if the status has changed before updating it prevents unnecessary operations and is an important enhancement for performance and correctness.

    8
    Performance
    Store the result of b.getStatus() in a variable to avoid redundant function calls

    Instead of calling b.getStatus() multiple times, store the result in a variable to avoid
    redundant function calls and improve readability.

    pkg/lockservice/lock_table_allocator.go [209-212]

    +status := b.getStatus()
     logServiceStatus("set restart lock service",
         serviceID,
    -    b.getStatus())
    +    status)
     b.setStatus(pb.Status_ServiceLockWaiting)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Storing the result of b.getStatus() in a variable reduces redundant function calls and improves code efficiency and readability.

    7
    Best practice
    Use logger.Warn instead of logger.Error for logging non-critical issues

    Use logger.Warn instead of logger.Error for logging abnormal lock service status, as it is
    more appropriate for unexpected but non-critical issues.

    pkg/lockservice/lock_table_keeper.go [214-216]

    -getLogger().Error("tn has abnormal lock service status",
    +getLogger().Warn("tn has abnormal lock service status",
         zap.String("serviceID", k.serviceID),
         zap.String("status", k.service.getStatus().String()))
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using logger.Warn instead of logger.Error is a good practice for logging non-critical issues, aligning the severity level with the actual impact.

    6

    @mergify mergify bot requested a review from fengttt June 24, 2024 11:13
    @mergify mergify bot merged commit a9b76fa into matrixorigin:main Jun 25, 2024
    17 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Review effort [1-5]: 3 size/M Denotes a PR that changes [100,499] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants