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

filter deletion in compaction filter #344

Merged
merged 8 commits into from
Aug 7, 2023

Conversation

tabokie
Copy link
Member

@tabokie tabokie commented Aug 1, 2023

And delay the buffer initialization of writable file to first actual write.

Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
@@ -195,7 +195,8 @@ class WritableFileWriter {
TEST_SYNC_POINT_CALLBACK("WritableFileWriter::WritableFileWriter:0",
reinterpret_cast<void*>(max_buffer_size_));
buf_.Alignment(writable_file_->GetRequiredBufferAlignment());
buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
// Removed to reduce WAL writer memory usage.
// buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it affect raftstore v1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can keep the code and use a smaller max_buffer_size in raftstore v2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation is delayed to actual write, which I think is good for both v1 and v2. Because the initialization is also on critical path, moving it elsewhere amortizes the overhead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. How about skipping alloc is max_buffer_size_ == 0?

Suggested change
// buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
if max_buffer_size_ != 0 {
buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@tabokie tabokie Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot this flag works for both WAL and SST. So we can't simply change the config.

Signed-off-by: tabokie <xy.tao@outlook.com>
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@@ -195,7 +195,8 @@ class WritableFileWriter {
TEST_SYNC_POINT_CALLBACK("WritableFileWriter::WritableFileWriter:0",
reinterpret_cast<void*>(max_buffer_size_));
buf_.Alignment(writable_file_->GetRequiredBufferAlignment());
buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
// Removed to reduce WAL writer memory usage.
// buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. How about skipping alloc is max_buffer_size_ == 0?

Suggested change
// buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
if max_buffer_size_ != 0 {
buf_.AllocateNewBuffer(std::min((size_t)65536, max_buffer_size_));
}

@@ -212,6 +217,21 @@ class CompactionFilter : public Customizable {
skip_until);
}

// This interface is reserved for TiKV's region range filter. Only this
// interface can accept `value_type=kTypeDeletion`.
virtual Decision UnsafeFilter(int level, const Slice& key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is possible to add a test case to show its usage?

Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
This reverts commit ba85ffa.

Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie tabokie merged commit de47e8e into tikv:6.29.tikv Aug 7, 2023
3 checks passed
@tabokie tabokie deleted the fix-compact-range branch August 7, 2023 10:03
v01dstar pushed a commit to v01dstar/rocksdb that referenced this pull request Feb 21, 2024
And delay the buffer initialization of writable file to first actual write.

---------

Signed-off-by: tabokie <xy.tao@outlook.com>
v01dstar pushed a commit to v01dstar/rocksdb that referenced this pull request Feb 22, 2024
compaction_filter: add bottommost_level into context (tikv#160)

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

add range for compaction filter context (tikv#192)

* add range for compaction filter context

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

allow no_io for VersionSet::GetTableProperties (tikv#211)

* allow no_io for VersionSet::GetTableProperties

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

expose seqno from compaction filter and iterator (tikv#215)

This PR supports to access `seqno` for every key/value pairs in compaction filter or iterator.
It's helpful to enhance GC in compaction filter in TiKV.

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

allow to query DB stall status (tikv#226)

This PR adds a new property is-write-stalled to query whether the column family is in write stall or not.

In TiKV there is a compaction filter used for GC, in which DB::write is called. So if we can query whether the DB instance is stalled or not, we can skip to create more compaction filter instances to save some resources.

Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

Fix compatibilty issue with Titan

Signed-off-by: v01dstar <yang.zhang@pingcap.com>

filter deletion in compaction filter (tikv#344)

And delay the buffer initialization of writable file to first actual write.

---------

Signed-off-by: tabokie <xy.tao@outlook.com>

Adjustments for compaptibilty with 8.10.facebook

Signed-off-by: v01dstar <yang.zhang@pingcap.com>

Adjust tikv related changes with upstream

Signed-off-by: v01dstar <yang.zhang@pingcap.com>
v01dstar pushed a commit to v01dstar/rocksdb that referenced this pull request May 29, 2024
And delay the buffer initialization of writable file to first actual write.

---------

Signed-off-by: tabokie <xy.tao@outlook.com>
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