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

update delete by deltaloc #17246

Merged

Conversation

jiangxinmeng1
Copy link
Contributor

@jiangxinmeng1 jiangxinmeng1 commented Jul 1, 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 #17239

What this PR does / why we need it:

Update delete by deltaloc. Rollback and retry if the object is soft deleted


PR Type

Bug fix, Tests


Description

  • Added HasDropIntentLocked method in BaseEntryImpl to check for drop intent.
  • Added TestDeltaLocation test case to validate soft delete and retry logic.
  • Enhanced TryDeleteByDeltaloc to check for drop intent before proceeding.
  • Updated PrepareCommit in DeltalocChain to include retry logic if there are new deletes or the object is soft deleted.

Changes walkthrough 📝

Relevant files
Enhancement
base.go
Add method to check for drop intent in BaseEntryImpl         

pkg/vm/engine/tae/catalog/base.go

  • Added HasDropIntentLocked method to check for drop intent.
+8/-0     
Tests
db_test.go
Add test case for delta location and soft delete                 

pkg/vm/engine/tae/db/test/db_test.go

  • Added TestDeltaLocation test case.
  • Tested soft delete and retry logic.
  • +47/-0   
    Bug fix
    base.go
    Add drop intent check in TryDeleteByDeltaloc                         

    pkg/vm/engine/tae/tables/base.go

    • Added check for drop intent in TryDeleteByDeltaloc.
    +3/-0     
    mvcc.go
    Add retry logic for new deletes and soft deleted objects in
    PrepareCommit

    pkg/vm/engine/tae/tables/updates/mvcc.go

  • Added retry logic if there are new deletes or the object is soft
    deleted in PrepareCommit.
  • +5/-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 TryDeleteByDeltaloc method in base.go now checks if the object has a drop intent before proceeding with the delete. Ensure that this logic correctly handles all edge cases related to object deletion states.
    Retry Logic:
    The PrepareCommit method in mvcc.go has been updated to include retry logic based on new deletes or soft deletion state. It's crucial to verify that this logic does not introduce any race conditions or deadlocks under high concurrency.

    @mergify mergify bot requested a review from sukki37 July 1, 2024 08:42
    @mergify mergify bot added the kind/bug Something isn't working label Jul 1, 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 bug
    Add a return value to the conditional return statement to match the expected function signature

    The new code checks if a block has a drop intent and returns immediately if true. However,
    the return statement does not specify what should be returned. This will cause a
    compilation error because the function expects a return value. To fix this, return the
    appropriate value based on the function's expected return type.

    pkg/vm/engine/tae/tables/base.go [794-796]

     if blk.meta.HasDropIntentLocked() {
    -    return
    +    return false, nil
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies a potential compilation error due to a missing return value and provides an appropriate fix based on the function's expected return type.

    10
    Possible issue
    Remove the redundant transaction commit to prevent double committing

    The test function TestDeltaLocation commits the transaction twice, which can lead to
    unexpected behavior or errors. The second commit should be removed to ensure the
    transaction is only committed once after all operations are completed.

    pkg/vm/engine/tae/db/test/db_test.go [6115-6116]

    -txn.Commit(ctx)
     assert.Error(t, err)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies and resolves the issue of double committing a transaction, which can lead to unexpected behavior or errors.

    9
    Enhancement
    Modify the function to handle potential errors returned by HasDropIntent

    The function HasDropIntentLocked retrieves the latest node and checks for a drop intent.
    However, it should handle the case where un.HasDropIntent() might also return an error
    along with the boolean value. This ensures that any error during the check is properly
    handled and not ignored.

    pkg/vm/engine/tae/catalog/base.go [219-223]

     un := be.GetLatestNodeLocked()
     if un == nil {
         return false
     }
    -return un.HasDropIntent()
    +hasIntent, err := un.HasDropIntent()
    +if err != nil {
    +    return false // or handle error appropriately
    +}
    +return hasIntent
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves error handling by ensuring that any errors returned by HasDropIntent are properly managed, enhancing the robustness of the function.

    8
    Best practice
    Add error handling for logging statements in the PrepareCommit function

    The logging statements in the PrepareCommit function should include error handling for the
    logger. This ensures that any issues during logging do not go unnoticed, which can be
    critical in debugging.

    pkg/vm/engine/tae/tables/updates/mvcc.go [658-662]

    -logutil.Infof("retry delete, there're new deletes in obj %v", d.mvcc.meta.ID.String())
    -logutil.Infof("retry delete, obj %v is soft deleted", d.mvcc.meta.ID.String())
    +if err := logutil.Infof("retry delete, there're new deletes in obj %v", d.mvcc.meta.ID.String()); err != nil {
    +    return err
    +}
    +if err := logutil.Infof("retry delete, obj %v is soft deleted", d.mvcc.meta.ID.String()); err != nil {
    +    return err
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: While adding error handling for logging statements is a good practice, it is not crucial for the functionality of the PrepareCommit function. The suggestion improves code quality but addresses a minor issue.

    6

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jul 1, 2024
    @mergify mergify bot merged commit 4f31a9d into matrixorigin:1.2-dev Jul 5, 2024
    17 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 kind/bug Something isn't working Review effort [1-5]: 3 size/S Denotes a PR that changes [10,99] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    4 participants