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 duplication entry error reported by DN #15126

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

triump2020
Copy link
Contributor

@triump2020 triump2020 commented Mar 23, 2024

What type of PR is this?

  • API-change
  • [ x] BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #14895 #14880 #14562 #14405 #13541 #14548 #13872

What this PR does / why we need it:

  1. Fix duplicaction entry error reported by DN
  • transfer persisted deletes, otherwise , dup still repro when deletes a large amount of data.

@triump2020 triump2020 changed the title fix dup reported by DN fix duplication entry error reported by DN Mar 23, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 23, 2024
@matrix-meow
Copy link
Contributor

@triump2020 Thanks for your contributions!

Pull Request Review:

Title and Body:

The title of the pull request is clear and concise, indicating that it aims to fix a duplication entry error reported by DN. The body of the pull request provides information about the type of PR (BUG), the related issues, and the purpose of the changes made in the PR. It mentions fixing the duplication entry error reported by DN.

Changes Made:

  1. In the antidepend.go file:

    • The addition of a comment explaining the purpose of the conflictSet field.
  2. In the table.go file:

    • The TransferDeletes function now includes an additional parameter ts which is used in subsequent function calls.
    • The recurTransferDelete function now includes the ts parameter to check if a block has been soft deleted and committed before a specific timestamp.
    • The TransferDeleteNode function now includes the ts parameter to transfer persisted deletes to the target block.
    • The TransferDeleteRows function now includes the ts parameter to handle transferred deletes based on a specific timestamp.

Suggestions for Improvement:

  1. Missing Error Handling:

    • Ensure that error handling is consistent and thorough throughout the codebase. Check for potential error scenarios and handle them appropriately to prevent unexpected behavior.
  2. Consistent Parameter Naming:

    • Maintain consistency in parameter naming across functions to improve code readability and maintainability.
  3. Optimization:

    • Consider refactoring the code to reduce redundancy and improve code efficiency where possible. Look for opportunities to streamline the logic and make the code more concise.
  4. Security Concerns:

    • Ensure that the timestamp (ts) parameter is properly validated and sanitized to prevent any potential security vulnerabilities related to timestamp manipulation.
  5. Documentation:

    • Add or update relevant comments and documentation to explain the purpose and functionality of the added or modified code sections. This will help developers understand the code better.
  6. Testing:

    • Verify that the changes made in the pull request are adequately tested to ensure they function as expected and do not introduce regressions.

By addressing the suggestions mentioned above, the codebase can be improved in terms of readability, maintainability, and security. It is essential to thoroughly review and test the changes before merging them into the main codebase.

@matrix-meow matrix-meow added size/M Denotes a PR that changes [100,499] lines and removed size/S Denotes a PR that changes [10,99] lines labels Mar 24, 2024
@mergify mergify bot merged commit 9b73c78 into matrixorigin:main Apr 16, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants