8000 refactor(storage): no default sstable info by wenym1 · Pull Request #21845 · risingwavelabs/risingwave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor(storage): no default sstable info #21845

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 14, 2025
Merged

refactor(storage): no default sstable info #21845

merged 1 commit into from
May 14, 2025

Conversation

wenym1
Copy link
Contributor
@wenym1 wenym1 commented May 13, 2025 •
edited
Loading

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Fields in SstableInfo is important for data correctness, and we should fill them carefully. Having Default impl for SstableInfo can easily implicitly fill in unexpected default value.

In this PR, we will remove the derive(Default) for SstableInfo when we are not in test build. Current usages of the default implementation are removed and changed accordingly. When dealing with OverlapStrategy, we used to pass the whole SstableInfo, but actually only its key_range is used. We will change to only pass the KeyRange.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

@wenym1 Graphite App
Copy link
Contributor Author
wenym1 commented May 13, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@wenym1 wenym1 marked this pull request as ready for review May 13, 2025 08:26
@wenym1 wenym1 requested review from hzxa21, Li0k and zwang28 May 13, 2025 08:38
@wenym1 wenym1 mentioned this pull request May 13, 2025
Merged
8 tasks
@wenym1 wenym1 force-pushed the yiming/no-default-sstable-info branch from 077a7aa to fae67c1 Compare May 13, 2025 09:28
@wenym1 wenym1 added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit 0966224 May 14, 2025
31 of 32 checks passed
@wenym1 wenym1 deleted the yiming/no-default-sstable-info branch May 14, 2025 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwang28 zwang28 zwang28 approved these changes

@hzxa21 hzxa21 Awaiting requested review from hzxa21

@Li0k Li0k Awaiting requested review from Li0k

Assignees
No one assigned
Labels
ci/run-e2e-single-node-tests ci/run-e2e-test-other-backends type/refactor Type: Refactoring.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wenym1 @zwang28
0