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

add support rollback running txn to 1.2 #18775

Merged

Conversation

zhangxu19830126
Copy link
Contributor

@zhangxu19830126 zhangxu19830126 commented Sep 13, 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 #4083

What this PR does / why we need it:

add support rollback running txn


PR Type

Enhancement, Tests


Description

  • Simplified the close method in column_cache.go by removing the locking mechanism and returning nil.
  • Introduced a runningSQL atomic boolean in operator.go to track the execution state of SQL transactions.
  • Enhanced Commit and Rollback methods to prevent operations if SQL is currently running.
  • Updated EnterRunSql and ExitRunSql methods to modify the runningSQL state.
  • Added comprehensive tests in operator_test.go to verify the new behavior of transaction operators, including checks for running SQL, lock skipping, and lock table presence.

Changes walkthrough 📝

Relevant files
Enhancement
column_cache.go
Simplify column cache close method                                             

pkg/incrservice/column_cache.go

  • Removed locking mechanism in the close method.
  • Simplified the close method to return nil.
  • +1/-3     
    operator.go
    Add state tracking for running SQL transactions                   

    pkg/txn/client/operator.go

  • Added runningSQL atomic boolean to track SQL execution state.
  • Added checks in Commit and Rollback to prevent operations during
    running SQL.
  • Updated EnterRunSql and ExitRunSql to modify runningSQL state.
  • +11/-0   
    Tests
    operator_test.go
    Add tests for transaction operator enhancements                   

    pkg/txn/client/operator_test.go

  • Added tests to ensure Commit and Rollback cannot be performed during
    running SQL.
  • Added tests for lock skipping and lock table presence.
  • +103/-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: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The use of util.GetLogger().Fatal() in Commit and Rollback methods may abruptly terminate the program. Consider using a more graceful error handling approach.

    Duplicate Code
    The error message "commit on running txn" is used for both Commit and Rollback methods. Consider using a more specific error message for Rollback.

    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Sep 13, 2024
    @mergify mergify bot requested a review from sukki37 September 13, 2024 06:34
    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 bug
    Correct error message for illegal rollback operation

    The error message for rollback is incorrect. It mentions "commit" instead of
    "rollback". Update the error message to accurately reflect the operation being
    performed.

    pkg/txn/client/operator.go [560-562]

     if tc.runningSQL.Load() {
    -    util.GetLogger().Fatal("commit on running txn")
    +    util.GetLogger().Fatal("Cannot rollback while SQL is running. Ensure all SQL operations are completed before rolling back.")
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion corrects a misleading error message, which is crucial for accurate debugging and understanding of the code's behavior, addressing a potential bug.

    9
    Error handling
    Return an error instead of terminating the program for better error handling

    Instead of using Fatal, which will terminate the program, consider returning an
    error. This allows the caller to handle the error gracefully and potentially recover
    from the situation.

    pkg/txn/client/operator.go [521-523]

     if tc.runningSQL.Load() {
    -    util.GetLogger().Fatal("commit on running txn")
    +    return fmt.Errorf("cannot commit while SQL is running: ensure all SQL operations are completed before committing")
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances error handling by allowing the program to handle errors gracefully instead of terminating, which is a significant improvement in robustness and flexibility.

    8
    Enhancement
    Improve error message clarity for illegal commit operation

    Consider using a more descriptive error message in the Fatal call. Instead of a
    generic message, provide more context about why committing during a running SQL
    operation is not allowed.

    pkg/txn/client/operator.go [521-523]

     if tc.runningSQL.Load() {
    -    util.GetLogger().Fatal("commit on running txn")
    +    util.GetLogger().Fatal("Cannot commit while SQL is running. Ensure all SQL operations are completed before committing.")
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the clarity of the error message, providing more context for the developer, which enhances maintainability and debugging.

    7
    Best practice
    Improve method naming for better code readability

    Consider using a more specific method name than Load() for the runningSQL atomic
    boolean. A method name like IsRunning() would be more descriptive and improve code
    readability.

    pkg/txn/client/operator.go [521]

    -if tc.runningSQL.Load() {
    +if tc.runningSQL.IsRunning() {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While the suggestion improves readability, it requires a change in the atomic library or wrapper, which may not be feasible or necessary if the method is standard.

    5

    @mergify mergify bot merged commit e74f4b9 into matrixorigin:1.2-dev Sep 13, 2024
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants