SST Partitioner interface that allows to split SST files #6957

koldat · 2020-06-08T17:14:54Z

SST Partitioner interface that allows to split SST files during compactions.

It basically instruct compaction to create a new file when needed. When one is using well defined prefixes and prefixed way of defining tables it is good to define also partitioning so that promotion of some SST file does not cover huge key space on next level (worst case complete space).

yiwu-arbug

Thanks! The PR is highly overlap with #5201, which I leave behind for long while (sorry!) I'm closing mine and hope this one can get merged.

include/rocksdb/sst_partitioner.h

db/compaction/compaction_job.cc

db/compaction/compaction.cc

db/compaction/compaction_job.cc

include/rocksdb/sst_partitioner.h

yiwu-arbug · 2020-06-16T00:18:39Z

include/rocksdb/sst_partitioner.h

+  // Called with key that is right after the key that was stored into the SST
+  // Returns true of partition boundary was detected and compaction should
+  // create new file.
+  virtual bool ShouldPartition(const Slice& key) = 0;


shall this method also take current output file size as parameter, so the partitioner can also split SST by size?

Split by size is functionality that compaction knows and there is a lot of options around it. Do we really want to introduce another way? Isn't it confusing? Main purpose of this change is to split based on data.

For our use case splitting SST is just a heuristic, not a mandatory. I want to be more flexible in that I can do "split SST based on key if the file is not too small or too large, otherwise override the behavior by size".

Is this method meant to tell you where it is safe to partition -- like it is okay to partition between data sets -- or meant to say "you really should not partition now"? If it is the latter, should the logic be reversed (ShouldNotPartition)?

It mean to say "please partition now".

It is "please partition now". I have put there two parameters to make it easier (removed reset)

db/compaction/compaction.h

yiwu-arbug · 2020-06-16T23:34:02Z

db/compaction/compaction_job.cc

@@ -945,6 +950,10 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
        key, value, ikey.sequence, ikey.type);
    sub_compact->num_output_records++;

+    if (partitioner.get() != nullptr) {
+      partitioner->Reset(key);


Can we merge the Reset and ShouldPartition call into one method call? (echoing the same comment from #5201 (comment))

It is possible, but then the loop will need to copy previous key. Right now the partitioner can remember only the bit it needs. It is hard to say if that copy will take more time, but I think virtual call is pretty cheap compared to memory copy of X number of bytes. So should I do that?

good point about memcopy. But it looks like in the current approach the Reset needs a memcopy as well. The key is a slice obtain from c_iter, whose lifetime is ended after c_iter->Next() is called. The partitioner needs to make memcopy to keep it. I'm not sure why ASAN test don't complain.

In case I didn't misread the code and memcopy is inevitable, I think we can change to the following flow:

for KV in c_iter { ... // process KV ... // check other SST partition condition if (partitioner->ShouldPartition(next_key)) { output_file_ended = true; } if (output_file_ended) { ... // partition SST partitioner->Reset(next_key); } }

That way the partitioner is invoke (#keys + #files) times, and it can decide on its own whether to memcopy keys per key or per file.

reset removed and should partition is with two arguments

koldat · 2020-06-17T06:20:45Z

@yiwu-arbug thanks for looking on the PR.

mrambacher · 2020-06-18T19:23:33Z

include/rocksdb/sst_partitioner.h

+  // Called with key that is right after the key that was stored into the SST
+  // Returns true of partition boundary was detected and compaction should
+  // create new file.
+  virtual bool ShouldPartition(const Slice& key) = 0;


Is this method meant to tell you where it is safe to partition -- like it is okay to partition between data sets -- or meant to say "you really should not partition now"? If it is the latter, should the logic be reversed (ShouldNotPartition)?

include/rocksdb/sst_partitioner.h

java/rocksjni/sst_partitioner.cc

include/rocksdb/sst_partitioner.h

mrambacher · 2020-06-18T19:52:28Z

include/rocksdb/sst_partitioner.h

+  virtual bool ShouldPartition(const Slice& key) = 0;
+
+  // Called for key that was stored into the SST
+  virtual void Reset(const Slice& key) = 0;


I am not sure what this method does based on the comment. Also Reset is too close of a name to reset for std::unique_ptr. Can we pick a more descriptive name?

reset removed.

mrambacher · 2020-06-18T19:59:05Z

include/rocksdb/sst_partitioner.h

+  virtual const char* Name() const = 0;
+};
+
+extern SstPartitionerFactory* NewSstPartitionerFixedPrefixFactory(


I have been introducing into classes that have potentially multiple implementations and loading a "CreateFromString" static method. You might want to check out how that is being done and see if something similar would work for you (Env::LoadEnv is a comparable method that I hope to deprecate). I am hoping to introduce the exact same CreateFromString method signature into many classes (as soon as the PRs are reviewed).

mrambacher · 2020-06-18T20:01:19Z

include/rocksdb/sst_partitioner.h

+#include <memory>
+#include <string>
+
+#include "rocksdb/rocksdb_namespace.h"


Should this code be limited by ROCKSDB_LITE or not? I do not know what the plans for LITE is or not and if this is necessary in LITE mode or should be skipped.

I do not know. Functionality is like 20 lines of code. Please let me know if I should not include in LITE

java/rocksjni/options.cc

mrambacher · 2020-06-18T20:14:27Z

db/compaction/compaction_job.cc

+      return false;
+    }
+    Slice key_fixed(key.data_, std::min(key.size_, len_));
+    return key_fixed.compare(last_key_) != 0;


Shouldn't this be based on a Comparator rather than raw bytes?

We compare prefix (number of bytes) for equality. I do not see much benefit to use comparator for this partitioner.

adamretter · 2020-06-22T18:30:31Z

@koldat @mrambacher when you guys are happy with the C++ API and functionality, let me know and I will review the Java stuff :-)

koldat · 2020-06-23T07:11:54Z

I have rebased to master head and squashed all changesets to make it clean. I have also added current file size to ShouldPartition that I have missed.

include/rocksdb/sst_partitioner.h

db/compaction/sst_partitioner.cc

yiwu-arbug · 2020-06-26T19:59:18Z

include/rocksdb/sst_partitioner.h

+
+  // Returns true if partition boundary was detected and compaction should
+  // create new file.
+  virtual bool ShouldPartition(const Slice& last_user_key,


Shall we create a new options struct for ShouldPartition parameter, so that the interface is extendable?

I am not sure I understand. We have the Context there already. If you want to pass struct then we should place there also prev and current key there. Is that what you mean? I have also fixed documentation to make the purpose easier to understand.

Yes, that's what I mean. It makes the API easier to extend later.

I have changed to
virtual PartitionerResult ShouldPartition(const PartitionerRequest& request) = 0;

db/compaction/compaction_job.cc

options/options.cc

db/compaction/compaction_job.cc

koldat · 2020-07-02T13:20:07Z

I have rebased and put latest review changes. I have changed the signature to:
virtual PartitionerResult ShouldPartition( const PartitionerRequest& request) = 0;
Result is enum where we can add later kPartitionMaybe, for now there is no/must. Request has same arguments as before, but is extendable (additional fields can be added without API change).

yiwu-arbug

LGTM. Anyone from the rocksdb team can do further review and merge? Thanks much.

yiwu-arbug · 2020-07-06T22:32:04Z

@siying @ajkr

siying

Maybe as a follow up, but I hope this feature is added to crash_test. Start with adding an option in db_stress and add to the crash_test parameter list.

siying · 2020-07-09T00:47:01Z

include/rocksdb/options.h

+  // If non-nullptr, use the specified factory for a function to determine the
+  // partitioning of sst files. This helps compaction to split the files
+  // on interesting boundaries (key prefixes) to make propagation of sst
+  // files less write amplifying (covering the whole key space).


Can we comment and say it's a experimental feature for now?

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2020-07-09T18:59:33Z

@koldat if you rebase against master and add a comment for the feature is experimental, we can try to merge it.

siying

Forward some our internal static analysis warnings. It will be good to address them.

siying · 2020-07-09T20:08:47Z

db/compaction/sst_partitioner.cc

+ public:
+  SstPartitionerFixedPrefix(size_t len) : len_(len) {}
+
+  virtual ~SstPartitionerFixedPrefix(){};


db/compaction/sst_partitioner.cc:17:41: warning: extra ';' after member function definition virtual ~SstPartitionerFixedPrefix(){}; ^

and

db/compaction/sst_partitioner.cc:17:11: warning: '~SstPartitionerFixedPrefix' overrides a destructor but is not marked 'override' virtual ~SstPartitionerFixedPrefix(){}; ^ include/rocksdb/sst_partitioner.h:46:11: note: overridden virtual function is here virtual ~SstPartitioner(){}; ^

siying · 2020-07-09T20:12:32Z

db/compaction/sst_partitioner.cc

+  size_t len_;
+};
+
+class SstPartitionerFixedPrefixFactory : public SstPartitionerFactory {


"Struct and class definitions should either be defined in a header file, declared in a header file with the same base name and path as the definition file, or in an anonymous namespace to prevent possible shadowing of other classes."

siying · 2020-07-09T20:12:58Z

db/compaction/sst_partitioner.cc

+
+class SstPartitionerFixedPrefix : public SstPartitioner {
+ public:
+  SstPartitionerFixedPrefix(size_t len) : len_(len) {}


The linter asks us to add "explicit".

siying · 2020-07-09T20:13:25Z

db/compaction/sst_partitioner.cc

+
+std::shared_ptr<SstPartitionerFactory> NewSstPartitionerFixedPrefixFactory(
+    size_t prefix_len) {
+  return std::shared_ptr<SstPartitionerFactory>(


The linter asks us to use make_shared

siying · 2020-07-09T20:13:42Z

db/compaction/sst_partitioner.cc

+
+class SstPartitionerFixedPrefixFactory : public SstPartitionerFactory {
+ public:
+  SstPartitionerFixedPrefixFactory(size_t len) : len_(len) {}


Same here. add explicit.

siying · 2020-07-09T20:14:19Z

db/compaction/sst_partitioner.cc

+
+namespace ROCKSDB_NAMESPACE {
+
+class SstPartitionerFixedPrefix : public SstPartitioner {


"Struct and class definitions should either be defined in a header file, declared in a header file with the same base name and path as the definition file, or in an anonymous namespace to prevent possible shadowing of other classes."

yiwu-arbug · 2020-07-16T02:21:33Z

@koldat ping

koldat · 2020-07-17T12:22:04Z

@yiwu-arbug I'll do next week. Was on vacation, sorry.

facebook-github-bot · 2020-07-22T10:24:09Z

@koldat has updated the pull request. You must reimport the pull request before landing.

koldat · 2020-07-22T10:27:24Z

@siying I have done all review changes, squashed and rebased to master.

The only question I have is about crash test. You want me to write new test in db_stress using sst_partitioner? It would be quite big task, because we need to define first what should be outcome of such a test. The partitioning is always application specific. On the other hand the only impact on compaction is just "two lines" and in case it is not enabled there is no impact. Please let me know if I should go and create something.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2020-07-23T18:45:44Z

@koldat there can be multiple levels on the partitioned about what we can validate. From a minimal level, we can just apply a prefix partitioned, enable it in stress test by say 1/3 of the time, and watch it doesn't generate wrong query results or crash the service. That's already very helpful. Validating logic correctness is lower priority. Again, I still hope the stress test to be done, but can be done as a follow-up PR.

facebook-github-bot · 2020-07-23T21:43:36Z

@koldat has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2020-07-23T21:48:43Z

@koldat sorry but after rebasing the branch to latest master, it seems to show more errors. Can you help fix them?

…titioning function

facebook-github-bot · 2020-07-24T11:37:53Z

@koldat has updated the pull request. You must reimport the pull request before landing.

koldat · 2020-07-24T13:14:47Z

@siying I have removed your merge, rebased to master and fixed the build.

facebook-github-bot

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-07-25T00:39:53Z

This pull request has been merged in cd4592c.

facebook-github-bot · 2020-07-25T00:40:10Z

This pull request has been merged in cd4592c.

facebook-github-bot · 2020-07-25T00:40:57Z

This pull request has been merged in cd4592c.

siying · 2020-07-27T23:10:02Z

Thank you for your contribution!

…) (#184) Summary: SST Partitioner interface that allows to split SST files during compactions. It basically instruct compaction to create a new file when needed. When one is using well defined prefixes and prefixed way of defining tables it is good to define also partitioning so that promotion of some SST file does not cover huge key space on next level (worst case complete space). Pull Request resolved: facebook#6957 Reviewed By: ajkr Differential Revision: D22461239 fbshipit-source-id: 9ce07bba08b3ba89c2d45630520368f704d1316e Signed-off-by: Yi Wu <yiwu@pingcap.com> Co-authored-by: Tomas Kolda <koldat@gmail.com>

Summary: SST Partitioner interface that allows to split SST files during compactions. It basically instruct compaction to create a new file when needed. When one is using well defined prefixes and prefixed way of defining tables it is good to define also partitioning so that promotion of some SST file does not cover huge key space on next level (worst case complete space). Pull Request resolved: facebook#6957 Reviewed By: ajkr Differential Revision: D22461239 fbshipit-source-id: 9ce07bba08b3ba89c2d45630520368f704d1316e

facebook-github-bot added the CLA Signed label Jun 8, 2020

koldat force-pushed the sst_partitioner branch from 01200d4 to 84ff7c5 Compare June 9, 2020 12:43

yiwu-arbug mentioned this pull request Jun 16, 2020

Interface to customize compaction output file boundary #5201

Closed

yiwu-arbug reviewed Jun 16, 2020

View reviewed changes

db/compaction/compaction.h Show resolved Hide resolved

yiwu-arbug reviewed Jun 16, 2020

View reviewed changes

mrambacher reviewed Jun 18, 2020

View reviewed changes

adamretter self-requested a review June 22, 2020 19:36

koldat force-pushed the sst_partitioner branch from 0a36177 to 9a3e759 Compare June 23, 2020 06:33

koldat requested review from yiwu-arbug and mrambacher June 23, 2020 07:12

yiwu-arbug reviewed Jun 26, 2020

View reviewed changes

include/rocksdb/sst_partitioner.h Outdated Show resolved Hide resolved

yiwu-arbug reviewed Jun 26, 2020

View reviewed changes

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

yiwu-arbug reviewed Jun 26, 2020

View reviewed changes

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

yiwu-arbug reviewed Jun 26, 2020

View reviewed changes

db/compaction/compaction_job.cc Show resolved Hide resolved

yiwu-arbug reviewed Jun 26, 2020

View reviewed changes

options/options.cc Outdated Show resolved Hide resolved

yiwu-arbug reviewed Jul 1, 2020

View reviewed changes

db/compaction/compaction_job.cc Show resolved Hide resolved

koldat force-pushed the sst_partitioner branch from 4a53a24 to 87be22d Compare July 2, 2020 13:15

yiwu-arbug approved these changes Jul 6, 2020

View reviewed changes

siying reviewed Jul 9, 2020

View reviewed changes

facebook-github-bot reviewed Jul 9, 2020

View reviewed changes

siying reviewed Jul 9, 2020

View reviewed changes

koldat force-pushed the sst_partitioner branch from 87be22d to 2f488a4 Compare July 22, 2020 10:23

facebook-github-bot reviewed Jul 23, 2020

View reviewed changes

SST Partitioner interface that allows to split SST files based on par…

558354c

…titioning function

koldat force-pushed the sst_partitioner branch from 6fee1e9 to 558354c Compare July 24, 2020 11:37

facebook-github-bot reviewed Jul 24, 2020

View reviewed changes

facebook-github-bot closed this in cd4592c Jul 24, 2020

facebook-github-bot added the Merged label Jul 25, 2020

yiwu-arbug mentioned this pull request Aug 10, 2020

SST Partitioner interface that allows to split SST files (#6957) tikv/rocksdb#184

Merged

koldat mentioned this pull request Feb 23, 2022

NPE in Java_org_rocksdb_ColumnFamilyOptions_setSstPartitionerFactory #9622

Closed


		namespace ROCKSDB_NAMESPACE {

		class SstPartitionerFixedPrefix : public SstPartitioner {

SST Partitioner interface that allows to split SST files #6957

SST Partitioner interface that allows to split SST files #6957

Conversation

koldat commented Jun 8, 2020

yiwu-arbug 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

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

yiwu-arbug Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koldat commented Jun 17, 2020

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

Choose a reason for hiding this comment

adamretter commented Jun 22, 2020

koldat commented Jun 23, 2020

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

koldat commented Jul 2, 2020

yiwu-arbug left a comment

Choose a reason for hiding this comment

yiwu-arbug commented Jul 6, 2020

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Jul 9, 2020

siying 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

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

Choose a reason for hiding this comment

yiwu-arbug commented Jul 16, 2020

koldat commented Jul 17, 2020

facebook-github-bot commented Jul 22, 2020

koldat commented Jul 22, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Jul 23, 2020

facebook-github-bot commented Jul 23, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Jul 23, 2020

facebook-github-bot commented Jul 24, 2020

koldat commented Jul 24, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 25, 2020

facebook-github-bot commented Jul 25, 2020

facebook-github-bot commented Jul 25, 2020

siying commented Jul 27, 2020

yiwu-arbug Jun 17, 2020 •

edited

Loading