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 rollback create table #16369

Merged

Conversation

jiangxinmeng1
Copy link
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #16282

What this PR does / why we need it:

Fix rollback create table.
When rollback, the last MVCC node has already removed. Remove related name list node with table.FullName.

@CLAassistant
Copy link

CLAassistant commented May 24, 2024

CLA assistant check
All committers have signed the CLA.

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 24, 2024
@mergify mergify bot requested a review from sukki37 May 24, 2024 01:55
@mergify mergify bot added the kind/bug Something isn't working label May 24, 2024
@matrix-meow
Copy link
Contributor

@jiangxinmeng1 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it fixes an issue related to rollback create table.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #16282), and the reason for the PR (fixing rollback create table). This information helps in understanding the purpose of the changes.

Changes:

  1. In the RemoveEntry function in database.go:

    • The code snippet added is responsible for handling the removal of a table entry during rollback.
    • The code correctly identifies and deletes the related name node when rolling back a table creation.
    • Suggestion: Consider adding comments to explain the purpose of the added code for better code readability.
  2. In the db_test.go test file:

    • The TestRollbackCreateTable function tests the rollback scenario for creating tables.
    • The test creates, drops, and rolls back the creation of tables to simulate a rollback scenario.
    • Suggestion: Ensure that the test covers all relevant scenarios and edge cases related to table creation and rollback.

Suggestions for Improvement:

  1. Code Comments: Add comments to explain the logic and purpose of the added code in the RemoveEntry function for better code understanding by other developers.

  2. Test Coverage: Ensure that the test cases cover various scenarios related to table creation, deletion, and rollback to validate the functionality thoroughly.

  3. Code Optimization: Consider refactoring the code to improve readability and maintainability. For instance, extracting repetitive code into separate functions can enhance code reusability.

  4. Security Concerns: While the changes provided seem focused on fixing the rollback functionality, ensure that there are no security vulnerabilities introduced, especially when handling table creation and deletion operations.

  5. Consistency: Ensure consistency in naming conventions, formatting, and coding style throughout the codebase to maintain a clean and uniform code structure.

By addressing the suggestions mentioned above and ensuring comprehensive testing, the quality and reliability of the codebase can be improved. Additionally, conducting a thorough code review to catch any potential issues or bugs can further enhance the overall stability of the system.

@mergify mergify bot merged commit c480068 into matrixorigin:1.2-dev May 24, 2024
17 of 19 checks passed
@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
kind/bug Something isn't working 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