Change the compaction filter logic to let periodic compaction go through custom compaction filter, to gc expired data

[luyade]

Fix the issue that, many expired data in the bottommost level don't get garbage collected

What type of PR is this?

• bug
• feature
• enhancement

What problem(s) does this PR solve?

Issue(s) number:

#5438

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

• Unit test(positive and negative cases)
• Function test
• Performance test
• N/A

Affects:

• Documentation affected (Please add the label if documentation needs to be modified.)
• Incompatibility (If it breaks the compatibility, please describe it and add the label.）
• If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
• Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

[critical27]

However, during daily running, the only custom-compaction chance will most possibly be used by upper level data compaction, such as level0 => level1. So the default 30-days periodic compaction will go through the default minor compaction, without go through custom compaction filter. So the expired data will always be there.

I do agree what you said here, this is an issue. In theory, we could judge the input level and output level. For example, only >= L4, we use the custom compaction filter? But I didn't find way to do it in CompactionFilterFactory for now.

[luyade]

However, during daily running, the only custom-compaction chance will most possibly be used by upper level data compaction, such as level0 => level1. So the default 30-days periodic compaction will go through the default minor compaction, without go through custom compaction filter. So the expired data will always be there.

I do agree what you said here, this is an issue. In theory, we could judge the input level and output level. For example, only >= L4, we use the custom compaction filter? But I didn't find way to do it in CompactionFilterFactory for now.

Yes, we don't have a way to do compaction for only >=L4.

For now, according to my experience, as time proceeds, the space data get more and more larger with lots of expired data, which makes the performance very bad and also higher risk for single rocksdb instance with large data.

[critical27]

Actually, there indeed is a level, see the code in KVCompactionFilter

  bool Filter(int level,
              const rocksdb::Slice& key,
              const rocksdb::Slice& val,
              std::string*,
              bool*) const override {
    UNUSED(level);
    return kvFilter_->filter(spaceId_,
                             folly::StringPiece(key.data(), key.size()),
                             folly::StringPiece(val.data(), val.size()));
  }

We can modify this way

[luyade]

Actually, there indeed is a level, see the code in KVCompactionFilter

  bool Filter(int level,
              const rocksdb::Slice& key,
              const rocksdb::Slice& val,
              std::string*,
              bool*) const override {
    UNUSED(level);
    return kvFilter_->filter(spaceId_,
                             folly::StringPiece(key.data(), key.size()),
                             folly::StringPiece(val.data(), val.size()));
  }

We can modify this way

updated

[pengweisong]

1. Maybe we should check level here.
2. Besides, I think when level < 4, we should also do the custom compaction now and then, with low frequency.

[luyade]

1. Maybe we should check level here.
2. Besides, I think when level < 4, we should also do the custom compaction now and then, with low frequency.

But it is not possible to get the level here.

[pengweisong]

But in your code now, if the level < 4, filter function will also be called for each key, which is not a good way.

[luyade]

IMO, this is a tradeoff. Although this is not the perfect idea, it should be better to leave the expired data not-really handled. What do you think? If there is better idea, it is appreciated.

[critical27]

LGTM, thx~

[critical27]

CI failed because of compile, please fix it~

[luyade]

CI failed because of compile, please fix it~

Fixed.

[critical27]

The test case failed 😢 , probably related to the new gflag min_level_for_custom_filter, probably need to set it to 0 in test case.

[luyade]

The test case failed 😢 , probably related to the new gflag min_level_for_custom_filter, probably need to set it to 0 in test case.

Yes, I noticed that failure. Just updated again for the test case.

[critical27]

Good job~~~ Thx again