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 truncate #16989

Merged
merged 2 commits into from
Jun 19, 2024
Merged

fix truncate #16989

merged 2 commits into from
Jun 19, 2024

Conversation

jiangxinmeng1
Copy link
Contributor

@jiangxinmeng1 jiangxinmeng1 commented Jun 19, 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 #16987

What this PR does / why we need it:

comsume ErrInvalidTruncateLsn


PR Type

Bug fix


Description

  • Added logging to indicate the start of the truncate process.
  • Introduced error handling for ErrInvalidTruncateLsn using moerr.IsMoErrCode.
  • Implemented retry logic for the truncate operation with additional logging for retries.
  • Enhanced final log message to confirm successful truncation.

Changes walkthrough 📝

Relevant files
Bug fix
truncate.go
Enhance truncate log service with error handling and logging

pkg/vm/engine/tae/logstore/driver/logservicedriver/truncate.go

  • Added logging for the start of the truncate process.
  • Added error handling for ErrInvalidTruncateLsn using
    moerr.IsMoErrCode.
  • Implemented retry logic for truncation with enhanced logging.
  • Improved final log message to indicate successful truncation.
  • +16/-2   

    💡 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 No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The error handling for ErrInvalidTruncateLsn might not reset the error err to nil correctly if the condition truncatedLsn == lsn is not met, leading to unnecessary retries or a panic.
    Error Handling:
    The use of panic for handling errors in the retry logic could be replaced with more graceful error handling to avoid crashing the service.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 19, 2024
    @mergify mergify bot requested a review from sukki37 June 19, 2024 03:53
    @mergify mergify bot added the kind/bug Something isn't working label Jun 19, 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
    Best practice
    Use defer to ensure the cancel function is always called

    To ensure that the cancel function is always called, even if client.c.Truncate panics,
    consider using a defer statement right after the context is created.

    pkg/vm/engine/tae/logstore/driver/logservicedriver/truncate.go [77-79]

     ctx, cancel := context.WithTimeout(context.Background(), d.config.TruncateDuration)
    +defer cancel()
     err = client.c.Truncate(ctx, lsn)
    -cancel()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using defer for the cancel function is a best practice in Go to ensure it is called even if the function exits unexpectedly, which improves the robustness of the code.

    9
    Log the error before panicking to provide more context for debugging

    To improve error handling, consider logging the error before panicking, providing more
    context for debugging.

    pkg/vm/engine/tae/logstore/driver/logservicedriver/truncate.go [100-101]

     if err != nil {
    +    logutil.Errorf("LogService Driver: Failed to truncate logservice with lsn %d, error: %v", lsn, err)
         panic(err)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Logging errors before panicking is crucial for debugging and understanding the context of failures, making this a valuable improvement.

    8
    Maintainability
    Extract retry logic into a separate function to avoid redundancy

    To avoid redundant code, extract the retry logic into a separate function and call it in
    both places where retry is needed.

    pkg/vm/engine/tae/logstore/driver/logservicedriver/truncate.go [87-98]

     err = RetryWithTimeout(d.config.RetryTimeout, func() (shouldReturn bool) {
    +    return retryTruncate(client, lsn)
    +})
    +
    +func retryTruncate(client ClientType, lsn uint64) bool {
         logutil.Infof("LogService Driver: retry truncate, lsn %d err is %v", lsn, err)
         ctx, cancel := context.WithTimeout(context.Background(), d.config.TruncateDuration)
    +    defer cancel()
         err = client.c.Truncate(ctx, lsn)
    -    cancel()
         if moerr.IsMoErrCode(err, moerr.ErrInvalidTruncateLsn) {
    -        truncatedLsn := d.getLogserviceTruncate()
    -        if truncatedLsn == lsn {
    +        currentTruncatedLsn := d.getLogserviceTruncate()
    +        if currentTruncatedLsn == lsn {
                 err = nil
             }
         }
         return err == nil
    -})
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting the retry logic into a separate function is a good practice for maintainability and code clarity, especially when the logic is complex and repeated.

    7
    Enhancement
    Rename variable to improve readability

    To avoid potential confusion and improve readability, consider renaming the variable
    truncatedLsn to something more descriptive, such as currentTruncatedLsn.

    pkg/vm/engine/tae/logstore/driver/logservicedriver/truncate.go [81]

    -truncatedLsn := d.getLogserviceTruncate()
    +currentTruncatedLsn := d.getLogserviceTruncate()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Renaming truncatedLsn to currentTruncatedLsn could slightly improve readability by making the variable's purpose clearer, though it's a minor enhancement.

    5

    @mergify mergify bot merged commit 5809205 into matrixorigin:1.2-dev Jun 19, 2024
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working Review effort [1-5]: 3 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants