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 #17092

Merged
merged 2 commits into from
Jun 21, 2024
Merged

fix lock table move failed #17092

merged 2 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 setting restart lock service and group table moves in lock_table_allocator.go.
  • Modified method from popGroupTables to topGroupTables and added logging for service status and lock table binds in lock_table_keeper.go.
  • Introduced new logging functions for service status and binds move in log.go.
  • Added logging for service status changes and a new method topGroupTables in service.go.
  • Added a new test TestIssue3538 to cover the lock table move issue in service_test.go.

Changes walkthrough 📝

Relevant files
Enhancement
lock_table_allocator.go
Add logging for restart service and group table moves       

pkg/lockservice/lock_table_allocator.go

  • Added logging for setting restart lock service.
  • Added logging for group table moves when disabling group tables.
  • +5/-0     
    lock_table_keeper.go
    Modify group table method and add logging                               

    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 move   

    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 for group tables                           

    pkg/lockservice/service.go

  • Added logging for service status changes.
  • Added new method topGroupTables to retrieve the top group tables.
  • Added logging in canLockOnServiceStatus method.
  • +16/-3   
    Tests
    service_test.go
    Add test for lock table move issue                                             

    pkg/lockservice/service_test.go

    • Added new test TestIssue3538 to cover the lock table move issue.
    +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 log.go uses logger.Error instead of logger.Info. This might be unintentional as the function name and context suggest it should be logging at the info level.
    Redundant Logging:
    The logStatusChange function is called with the same status for both parameters in service.go. This might not be useful as it logs a status change where the status remains the same.
    Code Duplication:
    The logging functionality for bind moves and status changes is repeated in multiple places (lock_table_keeper.go and service.go). Consider refactoring to reduce duplication and improve maintainability.

    @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
    Log the status change before updating the status variable

    The logStatusChange function should log the status before it is changed to ensure the
    correct status transition is logged.

    pkg/lockservice/service.go [390]

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

    Why: The suggestion correctly identifies a potential issue where the status is logged after being changed, which could lead to incorrect or misleading log entries. Moving the log statement before the status change is a crucial fix for accurate debugging and logging.

    10
    Ensure that the setRestartService function call does not return an error in the test

    Add a check to ensure that the setRestartService function call in TestIssue3538 does not
    return an error.

    pkg/lockservice/service_test.go [1517]

    -alloc.setRestartService("s1")
    +require.NoError(t, alloc.setRestartService("s1"))
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion is very relevant as it ensures that errors are properly handled in tests, which is crucial for reliable and robust testing. Adding error checks can prevent silent failures and ensure that the test suite accurately reflects the state and behavior of the code.

    8
    Maintainability
    Return a copy of the group tables slice to prevent data races or unintended modifications

    In the topGroupTables function, return a copy of the group tables slice to avoid potential
    data races or unintended modifications.

    pkg/lockservice/service.go [406]

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

    Why: The suggestion correctly points out a best practice to avoid data races or unintended side effects by returning a copy of the slice. This is particularly important in concurrent environments and enhances the maintainability and safety of the code.

    7

    @mergify mergify bot merged commit 72b187e into matrixorigin:main Jun 21, 2024
    16 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.

    3 participants