Skip to content

Conversation

@rescrv
Copy link
Contributor

@rescrv rescrv commented Jun 28, 2025

Description of changes

This adds a test for PR #4972. It also makes garbage collection bail on
log contention.

Test plan

CI

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

N/A

This adds a test for PR #4972.  It also makes garbage collection bail on
log contention.
@propel-code-bot
Copy link
Contributor

Add Regression Test and Error Handling for Hung Garbage Collection (PR #4972)

This PR introduces a new integration test specifically designed for the scenario addressed in PR #4972 (potential hung garbage collection in a Kubernetes environment). Additionally, it modifies the garbage collection logic in manifest_manager.rs so that GC will now exit immediately upon any log contention error, rather than silently ignoring certain contention states.

Key Changes:
• Added rust/wal3/tests/test_k8s_integration_89_hung_gc.rs to reproduce and verify the bug fixed in PR #4972
• Modified error handling in ManifestManager::apply_garbage to return on any log contention error
• Introduced new test data files under rust/wal3/tests/test_k8s_integration_89_hung_gc/

Affected Areas:
• ManifestManager garbage collection code (rust/wal3/src/manifest_manager.rs)
• Integration tests for WAL3 (rust/wal3/tests/test_k8s_integration_89_hung_gc.rs)
• Test data for integration tests

This summary was automatically generated by @propel-code-bot

@github-actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@rescrv rescrv requested a review from sanketkedia June 28, 2025 17:14
@rescrv rescrv merged commit 3580dff into main Jun 29, 2025
56 of 58 checks passed
@rescrv rescrv deleted the rescrv/test-gc branch June 29, 2025 17:49
rescrv added a commit that referenced this pull request Jul 1, 2025
## Description of changes

This adds a test for PR #4972.  It also makes garbage collection bail on
log contention.

## Test plan

CI

- [X] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Documentation Changes

N/A
rescrv added a commit that referenced this pull request Jul 1, 2025
## Description of changes

Included PRs in order:

- **[TST]  Test for #4972 (#4983)**
- **[CLN] Fix dedup in get_collections_with_new_data. (#4974)**
- **[ENH] Implement three-phase garbage collection for WAL3 (#4984)**
- **[CLN] Remove err(Display) from wal3. (#4992)**
- **[ENH] Wire up garbage collector to do 3-phase GC. (#4987)**
- **[ENH]  Do not materialize all fragments to delete. (#5004)**

## Test plan

CI through main and then CI on the hotfix branch.

## Documentation Changes

N/A
Inventrohyder pushed a commit to Inventrohyder/chroma that referenced this pull request Aug 5, 2025
## Description of changes

This adds a test for PR chroma-core#4972.  It also makes garbage collection bail on
log contention.

## Test plan

CI

- [X] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Documentation Changes

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants