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 lock table move failed #17091

Merged
merged 3 commits into from
Jun 21, 2024
Merged

fix lock table move failed #17091

merged 3 commits into from
Jun 21, 2024

Conversation

iamlinjunhong
Copy link
Contributor

@iamlinjunhong iamlinjunhong commented Jun 21, 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/3538

What this PR does / why we need it:

fix lock table move failed


PR Type

Bug fix, Tests


Description

  • Added logging for various operations in lock table allocator, keeper, and service.
  • Changed method from popGroupTables to topGroupTables in lock table keeper.
  • Introduced new logging functions for service status and binds move.
  • Added a new test case to cover issue 3538.

Changes walkthrough 📝

Relevant files
Enhancement
lock_table_allocator.go
Add logging for lock table allocator operations                   

pkg/lockservice/lock_table_allocator.go

  • Added logging for setting restart lock service.
  • Added logging for moving group tables.
  • +5/-0     
    lock_table_keeper.go
    Enhance lock table keeper with logging and method change 

    pkg/lockservice/lock_table_keeper.go

  • Changed method from popGroupTables to topGroupTables.
  • Added logging for service status and lock table binds.
  • +6/-1     
    log.go
    Add logging functions for service status and binds             

    pkg/lockservice/log.go

  • Added new logging functions for service status and binds move.
  • Added helper function for logging lock table binds.
  • +49/-0   
    service.go
    Add logging and new method in service implementation         

    pkg/lockservice/service.go

  • Added logging for status changes and lock on service checks.
  • Introduced topGroupTables method.
  • +16/-3   
    Tests
    service_test.go
    Add test case for issue 3538                                                         

    pkg/lockservice/service_test.go

    • Added new test case for issue 3538.
    +63/-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 method logCanLockOnService in pkg/lockservice/log.go logs an error instead of info. This might be a typo or incorrect logging level.
    Code Redundancy:
    The method logStatusChange is called with the same status for both parameters in pkg/lockservice/service.go. This might not be useful and could be a mistake.

    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 21, 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
    Possible issue
    Return a copy of the group tables slice to avoid potential data races or unintended modifications

    The topGroupTables function should return a copy of the group tables slice to avoid
    potential data races or unintended modifications to the internal state.

    pkg/lockservice/service.go [411]

    -return g
    +return append([]pb.LockTable(nil), g...)
     
    Suggestion importance[1-10]: 8

    Why: This is a crucial suggestion for thread safety and data integrity. Returning a copy of the slice prevents external modifications from affecting the internal state, which is particularly important in concurrent environments.

    8
    Log the status change before updating the status to ensure accurate logging

    The logStatusChange function should log the status before it is changed to ensure the log
    accurately reflects the transition.

    pkg/lockservice/service.go [395]

    +logStatusChange(s.mu.status, status)
     s.mu.status = status
    -logStatusChange(s.mu.status, status)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion is correct in recommending that the log should capture the old and new status before the actual change. This ensures the log reflects the transition accurately.

    7
    Best practice
    Move the defer statement to the beginning of the function to ensure it is always executed

    In canLockOnServiceStatus, the defer logCanLockOnService() should be placed at the
    beginning of the function to ensure it is executed regardless of where the function
    returns.

    pkg/lockservice/service.go [319-322]

    +defer logCanLockOnService()
     if s.isStatus(pb.Status_ServiceLockEnable) {
         return true
     }
    -defer logCanLockOnService()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion is valid as moving the defer statement ensures that the logging function is always executed regardless of where the function exits, which is a good practice for consistent logging.

    6

    @mergify mergify bot requested a review from sukki37 June 21, 2024 13:26
    @mergify mergify bot merged commit 15d3373 into matrixorigin:1.2-dev Jun 21, 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