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 wrong metadata parsing in CompactOnExpiredCollector #1703

Merged
merged 4 commits into from
Aug 26, 2023

Conversation

PragmaTwice
Copy link
Member

The bug only affects users who enable the option ENABLE_NEW_ENCODING.

We fixed the metadata parsing (on expire time field) in CompactOnExpiredCollector.

Besides, we change the type of some timestamp from int to int64.

@PragmaTwice PragmaTwice requested a review from git-hulk August 25, 2023 12:07
git-hulk
git-hulk previously approved these changes Aug 25, 2023
git-hulk
git-hulk previously approved these changes Aug 25, 2023
@git-hulk git-hulk self-requested a review August 25, 2023 14:19
@git-hulk git-hulk dismissed their stale review August 25, 2023 14:21

Unit test failed

@PragmaTwice
Copy link
Member Author

Carry comment from @git-hulk in PragmaTwice#2 for better context:

RocksDB compaction allows to use of trivial move mode(move file into the next level instead of using compaction filter to generate the output file) when the compaction is not triggered by the manual way.

So after this fix, NeedCompact will return the correct result and then wait for the background compaction schedule(become no manual way so it allows the trivial move), so after we call the storage->Compact, it won't manually compact the moved file.

I fixed this issue by using force bottom-level compaction when triggering the manual compaction, it's our expected behavior.

See https://github.com/facebook/rocksdb/blob/ba597514309b686d8addb59616f067d5522186b7/db/db_impl/db_impl_compaction_flush.cc#L3358

@PragmaTwice
Copy link
Member Author

Thanks all!

@PragmaTwice PragmaTwice merged commit 6bcd387 into apache:unstable Aug 26, 2023
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.

3 participants