Skip to content

Update GC filter #22178

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

Merged
merged 10 commits into from
Jul 17, 2025
Merged

Update GC filter #22178

merged 10 commits into from
Jul 17, 2025

Conversation

LeftHandCold
Copy link
Contributor

@LeftHandCold LeftHandCold commented Jul 16, 2025

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 #21982

What this PR does / why we need it:

After removing the GC metadata, the files that were originally to be GCed may have leaks


PR Type

Bug fix


Description

  • Remove empty dropTS check from GC filter logic

  • Fix potential file leaks after GC metadata removal


Changes diagram

flowchart LR
  A["GC Filter Logic"] --> B["Remove dropTS.IsEmpty() Check"]
  B --> C["Prevent File Leaks"]
  C --> D["Improved GC Cleanup"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
exec_v1.go
Remove empty dropTS check from GC filter                                 

pkg/vm/engine/tae/db/gc/v3/exec_v1.go

  • Remove dropTS.IsEmpty() condition from object filtering logic
  • Simplify conditional check to prevent file leaks during GC
  • +2/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    21982 - Partially compliant

    Compliant requirements:

    • Prevent file leaks after GC metadata removal

    Non-compliant requirements:

    • GC can be reset offline by removing all GC metadata files and still work
    • GC can be reset online

    Requires further human verification:

    • Verification that offline GC reset functionality works correctly
    • Testing of online GC reset capability
    • Validation that file leaks are actually prevented in practice

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Change

    The removal of the dropTS.IsEmpty() condition changes the filtering logic significantly. Need to verify this doesn't introduce unintended side effects or break existing GC behavior for objects with empty drop timestamps.

    if (*transObjects)[name] == nil ||
    	(*transObjects)[name][tableIDs[i]] == nil {

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jul 16, 2025
    Copy link

    qodo-merge-pro bot commented Jul 16, 2025

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Remove redundant nil check

    *The condition checks if (transObjects)[name] is nil twice - once in the outer
    if and again in the inner if. This redundant check can be optimized by
    restructuring the logic to avoid the duplicate nil check.

    pkg/vm/engine/tae/db/gc/v3/exec_v1.go [261-267]

    -if (*transObjects)[name] == nil ||
    -	(*transObjects)[name][tableIDs[i]] == nil {
    -	if (*transObjects)[name] == nil {
    -		(*transObjects)[name] = make(map[uint64]*ObjectEntry)
    -	}
    +if (*transObjects)[name] == nil {
    +	(*transObjects)[name] = make(map[uint64]*ObjectEntry)
    +}
    +if (*transObjects)[name][tableIDs[i]] == nil {
     	object := &ObjectEntry{
     		stats:    &stats,
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that (*transObjects)[name] is checked for nil twice and proposes a valid refactoring that simplifies the logic and improves readability.

    Low
    • Update

    Copy link
    Contributor

    mergify bot commented Jul 17, 2025

    This pull request has been removed from the queue for the following reason: checks failed.

    The merge conditions cannot be satisfied due to failing checks:

    You may have to fix your CI before adding the pull request to the queue again.
    If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
    If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

    @mergify mergify bot merged commit 94bccea into matrixorigin:main Jul 17, 2025
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/enhancement Review effort 2/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants