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 condition to ignore delete booking if no transfer needed on 1.2-dev #16632

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

w-zr
Copy link
Contributor

@w-zr w-zr commented Jun 4, 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 #16631

What this PR does / why we need it:

Add a log statement to improve traceability when the transfer is not needed


PR Type

Bug fix


Description

  • Fixed a bug in pkg/vm/engine/disttae/txn_table.go where delete booking was not correctly ignored when transfer was not needed.
  • Changed the condition to check for !taskHost.DoTransfer() instead of taskHost.DoTransfer().

Changes walkthrough 📝

Relevant files
Bug fix
txn_table.go
Fix condition to ignore delete booking if no transfer needed

pkg/vm/engine/disttae/txn_table.go

  • Fixed logic to ignore delete booking if transfer is not needed.
  • Changed condition from if taskHost.DoTransfer() to if
    !taskHost.DoTransfer().
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @w-zr w-zr changed the title ignore delete booking if do not need transfer ignore delete booking if do not need transfer on 1.2-dev Jun 4, 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 Review 🔍

    ⏱️ Estimated effort to review [1-5]

    1, because the PR involves a simple one-line logical change in the condition. The change is straightforward and does not involve complex logic or multiple files.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    No

    🔒 Security concerns

    No

    @mergify mergify bot requested a review from sukki37 June 4, 2024 07:42
    @mergify mergify bot added the kind/bug Something isn't working label Jun 4, 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
    Traceability
    Add a log statement to improve traceability when the transfer is not needed

    Add a log statement before returning taskHost.commitEntry to provide better traceability
    and debugging information when the transfer is not needed.

    pkg/vm/engine/disttae/txn_table.go [2603-2604]

     if !taskHost.DoTransfer() {
    +    log.Println("Transfer not needed, returning commit entry")
         return taskHost.commitEntry, nil
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding a log statement is a valid suggestion for improving traceability and debugging. This would help in understanding the flow of execution, especially when the transfer is not performed.

    7

    @w-zr w-zr changed the title ignore delete booking if do not need transfer on 1.2-dev Fix condition to ignore delete booking if no transfer needed on 1.2-dev Jun 4, 2024
    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jun 4, 2024
    @mergify mergify bot merged commit 64bccfe into matrixorigin:1.2-dev Jun 4, 2024
    17 of 18 checks passed
    @w-zr w-zr deleted the fix-DoTransfer-during-merge branch June 4, 2024 10:23
    @aylei aylei mentioned this pull request Jun 5, 2024
    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]: 1 size/XS Denotes a PR that changes [1, 9] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants