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

compaction: Add tests for forced compactions #868

Closed
itsbilal opened this issue Aug 24, 2020 · 0 comments · Fixed by #945
Closed

compaction: Add tests for forced compactions #868

itsbilal opened this issue Aug 24, 2020 · 0 comments · Fixed by #945

Comments

@itsbilal
Copy link
Member

When files are marked for compaction (i.e with FileMetadata.MarkedForCompaction = true, something that RocksDB often does), and when no other higher-priority score based compaction exists, we end up generating a compaction out of the picked file(s).

The portion of code in compactionPickerByScore.pickAuto that does this is currently untested by unit tests. This can be an issue, as it could let bugs in that logic go ignored until they get caught by higher level tests or roachtests in Cockroach. An example of such a recent bug would be cockroachdb/cockroach#53121 .

Changing testdata/compaction_picker_target_level to allow for specifying sstables marked for compactions should be a fairly straightforward way to test this logic.

itsbilal added a commit to itsbilal/pebble that referenced this issue Sep 28, 2020
This change adds a test to test compaction picking logic where
MarkedForCompaction = true. Previously, this boolean was completely
untested, resulting in panics when Pebble was used on a previously-rocksdb
store directory, as RocksDB often set that boolean to true in the manifest.

Fixes cockroachdb#868.
itsbilal added a commit that referenced this issue Sep 28, 2020
This change adds a test to test compaction picking logic where
MarkedForCompaction = true. Previously, this boolean was completely
untested, resulting in panics when Pebble was used on a previously-rocksdb
store directory, as RocksDB often set that boolean to true in the manifest.

Fixes #868.
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 a pull request may close this issue.

1 participant