compaction: send file size and range segments to compaction partitioner context #341

YuJuncen · 2023-07-20T09:53:06Z

This PR added a fields describing the overlapped file info of "next level of output level" (i.e. output_level + 1) info to the partitioner factory if there is.

Signed-off-by: hillium <yujuncen@pingcap.com>

This reverts commit dde965f. Signed-off-by: hillium <yujuncen@pingcap.com>

Signed-off-by: hillium <yujuncen@pingcap.com>

tabokie · 2023-07-28T00:53:47Z

db/compaction/compaction.cc

@@ -577,6 +619,8 @@ std::unique_ptr<SstPartitioner> Compaction::CreateSstPartitioner() const {
  context.output_level = output_level_;
  context.smallest_user_key = smallest_user_key_;
  context.largest_user_key = largest_user_key_;
+  context.output_next_level_segments =
+      CreateSegmentsForLevel(output_level_ + 1);


Should consider start and end key.

The start key and end key of this compaction can be accessed during creating segments, could you describe more details about the advice?

tabokie · 2023-07-28T00:56:50Z

db/compaction/compaction.cc

+    if (cmp->Compare(iter->smallest_key, largest_user_key_) > 0) {
+      break;
+    }
+    segs.emplace_back(iter->fd.GetFileSize(), iter->largest_key);


I don't know why test work, but this doesn't copy string, the string will be destructed once iter is out of scope.

I'm not sure how to own a Slice, it looks like a pure reference type(The content is a const char*)... Should we use PinSlice here (Or are there other types presents a owned slice?)?

For this case, I'm guessing the slice should live as long as VersionStorageInfo. I'm not sure whether that is true, if I got wrong, perhaps it is needed to use a owned type.

You are probably right.

tabokie · 2023-07-28T00:57:52Z

include/rocksdb/sst_partitioner.h

@@ -62,6 +62,13 @@ class SstPartitioner {
  virtual bool CanDoTrivialMove(const Slice& smallest_user_key,
                                const Slice& largest_user_key) = 0;

+  struct Segment {


This is cumbersome to implement in ffi. You should use two separate vectors instead.

tabokie

Rest LG, you should test this out in TiKV before merge.

tabokie · 2023-07-28T05:05:28Z

db/compaction/compaction.cc

+    // The level shall be empty.
+    return std::vector<SstPartitioner::Segment>();
+  }
+  const auto files = vsi->LevelFilesBrief(in_level);


Suggested change

const auto files = vsi->LevelFilesBrief(in_level);

const auto& files = vsi->LevelFilesBrief(in_level);

tabokie · 2023-07-28T05:05:34Z

db/compaction/compaction.cc

+  }
+  const auto files = vsi->LevelFilesBrief(in_level);
+  const auto cmp = immutable_options()->user_comparator;
+  const auto bgn = std::lower_bound(


Suggested change

const auto bgn = std::lower_bound(

const auto start = std::lower_bound(

tabokie · 2023-07-28T05:07:39Z

db/compaction/compaction.cc

+      [cmp](FdWithKeyRange& fd, const Slice& slice) {
+        return cmp->Compare(fd.largest_key, slice) < 0;
+      });
+  const auto end = files.files + files.num_files;


Move this line up.

tabokie · 2023-07-28T05:08:13Z

db/compaction/compaction.cc

+  }
+
+  std::vector<SstPartitioner::Segment> segs;
+  segs.emplace_back(0, bgn->smallest_key);


This should only be added if there exists at least one file in range.

Signed-off-by: hillium <yujuncen@pingcap.com>

…-level-seg-on-compaction

Signed-off-by: hillium <yujuncen@pingcap.com>

db/compaction/compaction.cc

Co-authored-by: Xinye Tao <xy.tao@outlook.com> Signed-off-by: hillium <yujuncen@pingcap.com>

Connor1996

LGTM

Connor1996 · 2023-09-04T06:23:28Z

db/compaction/compaction.cc

@@ -566,6 +567,52 @@ std::unique_ptr<CompactionFilter> Compaction::CreateCompactionFilter(
      context);
 }

+std::pair<std::vector<Slice>, std::vector<uint64_t>>
+Compaction::CreateSegmentsForLevel(int in_level) const {


why call it in_level?

This creates the segment of the input level, so the argument is named in_level, perhaps a plain level might be fine. 🤔️

Signed-off-by: hillium <yujuncen@pingcap.com>

YuJuncen · 2023-09-07T02:42:44Z

@Connor1996 could you help to merge this?

…er context (tikv#341) compaction: send file size and range segments to compaction partitioner context Signed-off-by: hillium <yujuncen@pingcap.com>

YuJuncen added 7 commits July 14, 2023 13:44

WIP

10b6d92

Signed-off-by: hillium <yujuncen@pingcap.com>

added test

80a376c

Signed-off-by: hillium <yujuncen@pingcap.com>

make clippy happy

9cad5cc

Signed-off-by: hillium <yujuncen@pingcap.com>

fix tests

335cdba

Signed-off-by: hillium <yujuncen@pingcap.com>

format

dde965f

Signed-off-by: hillium <yujuncen@pingcap.com>

Revert "format"

8b38b8d

This reverts commit dde965f. Signed-off-by: hillium <yujuncen@pingcap.com>

run format

395e346

Signed-off-by: hillium <yujuncen@pingcap.com>

YuJuncen requested review from tonyxuqqi and tabokie July 26, 2023 05:15

tabokie reviewed Jul 28, 2023

View reviewed changes

YuJuncen added 7 commits July 28, 2023 16:57

address comments

84e26ae

Signed-off-by: hillium <yujuncen@pingcap.com>

apply format

28c7415

Signed-off-by: hillium <yujuncen@pingcap.com>

added some helper functions to context

f54a535

Signed-off-by: hillium <yujuncen@pingcap.com>

rename the interface

0c20c20

Signed-off-by: hillium <yujuncen@pingcap.com>

fix some problems about key formatting

41dbff7

Signed-off-by: hillium <yujuncen@pingcap.com>

Merge branch '6.29.tikv' of https://github.com/tikv/rocksdb into next…

76db577

…-level-seg-on-compaction

run formatter

9b17013

Signed-off-by: hillium <yujuncen@pingcap.com>

This was referenced Aug 21, 2023

added next level of output level SST's metadata for compactions tikv/rust-rocksdb#760

Merged

compaction_guard: split SST when detected possible huge compaction tikv/tikv#15379

Merged

remove a strange line

c6e8da6

Signed-off-by: hillium <yujuncen@pingcap.com>

tabokie approved these changes Aug 23, 2023

View reviewed changes

db/compaction/compaction.cc Outdated Show resolved Hide resolved

Update db/compaction/compaction.cc

9134efe

Co-authored-by: Xinye Tao <xy.tao@outlook.com> Signed-off-by: hillium <yujuncen@pingcap.com>

YuJuncen force-pushed the next-level-seg-on-compaction branch from d644dd8 to 9134efe Compare August 23, 2023 06:59

Connor1996 approved these changes Sep 4, 2023

View reviewed changes

rename in_level to level

666e823

Signed-off-by: hillium <yujuncen@pingcap.com>

Connor1996 merged commit e2f6ec7 into tikv:6.29.tikv Sep 7, 2023
2 checks passed

v01dstar mentioned this pull request Sep 29, 2024

Add size and range in compaction partitioner context #386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compaction: send file size and range segments to compaction partitioner context #341

compaction: send file size and range segments to compaction partitioner context #341

YuJuncen commented Jul 20, 2023

tabokie Jul 28, 2023

YuJuncen Jul 28, 2023

tabokie Jul 28, 2023

tabokie Jul 28, 2023

YuJuncen Jul 28, 2023

tabokie Jul 28, 2023

tabokie Jul 28, 2023

tabokie left a comment

tabokie Jul 28, 2023

tabokie Jul 28, 2023

tabokie Jul 28, 2023

tabokie Jul 28, 2023

Connor1996 left a comment

Connor1996 Sep 4, 2023

YuJuncen Sep 4, 2023

YuJuncen commented Sep 7, 2023

	const auto files = vsi->LevelFilesBrief(in_level);
	const auto& files = vsi->LevelFilesBrief(in_level);

	const auto bgn = std::lower_bound(
	const auto start = std::lower_bound(

compaction: send file size and range segments to compaction partitioner context #341

compaction: send file size and range segments to compaction partitioner context #341

Conversation

YuJuncen commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuJuncen commented Sep 7, 2023