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 merge panic #18762

Merged
merged 7 commits into from
Sep 12, 2024
Merged

Fix merge panic #18762

merged 7 commits into from
Sep 12, 2024

Conversation

XuPeng-SH
Copy link
Contributor

@XuPeng-SH XuPeng-SH commented Sep 12, 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 #18719 #18745 #18742

What this PR does / why we need it:

fix merge panic


PR Type

Bug fix


Description

  • Removed unused imports from the merger.go file to clean up the code.
  • Eliminated a section of code that checked for duplicate sort keys and logged a warning, which was unnecessary and could lead to a panic.

Changes walkthrough 📝

Relevant files
Bug fix
merger.go
Remove unused imports and duplicate sort key check             

pkg/vm/engine/tae/mergesort/merger.go

  • Removed unused import of logutil and zap.
  • Deleted a block of code that checked for duplicate sort keys and
    logged a warning.
  • +1/-17   

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Removed Warning
    The PR removes a block of code that checked for duplicate sort keys and logged a warning. This might impact the ability to detect and debug issues related to duplicate keys.

    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
    Error handling
    Add error handling for potential failures in writer preparation

    Consider adding error handling for the PrepareNewWriter() call. If this method can
    potentially return an error, it should be checked and handled appropriately.

    pkg/vm/engine/tae/mergesort/merger.go [167-169]

     if m.writer == nil {
    -    m.writer = m.host.PrepareNewWriter()
    +    var err error
    +    m.writer, err = m.host.PrepareNewWriter()
    +    if err != nil {
    +        return fmt.Errorf("failed to prepare new writer: %w", err)
    +    }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the PrepareNewWriter() call is crucial for robust code, as it ensures that any potential errors are caught and managed, preventing unexpected behavior or crashes.

    9
    Context handling
    Add a context check before preparing a new writer to handle potential cancellations

    Consider adding a context check before the PrepareNewWriter() call to ensure the
    operation hasn't been cancelled.

    pkg/vm/engine/tae/mergesort/merger.go [167-169]

     if m.writer == nil {
    +    if err := ctx.Err(); err != nil {
    +        return fmt.Errorf("context cancelled before preparing new writer: %w", err)
    +    }
         m.writer = m.host.PrepareNewWriter()
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a context check before the PrepareNewWriter() call is a good practice to handle potential cancellations, ensuring that resources are not unnecessarily allocated if the operation is no longer needed.

    8

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 12, 2024
    @sukki37 sukki37 merged commit d37fb88 into matrixorigin:main Sep 12, 2024
    15 of 17 checks passed
    @XuPeng-SH XuPeng-SH deleted the fix-merge-panic branch September 13, 2024 02:31
    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]: 2 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants