IngestFile: support ingest SST files generated by live DB #5602

zxylvlp · 2019-07-21T07:45:14Z

No description provided.

anand1976 · 2019-07-22T18:43:38Z

@zxylvlp Can you describe the use case for this feature?

zxylvlp · 2019-07-23T03:43:27Z

Feture Request: Merge two rocksdb instances

We want to implement a merge operation of two rocksdb instance in our distributed table store like hbase.

Firstly, we try repairdb, but we found that repairdb need same cf name with same cf id. It's not a strongly guarantee in rocksdb implements.

Then, we try to use ingest file, but ingest file need has table properties ingest-file.version and ingest-file.global_seq which not exists in normal sst.

So we add ingest-file.version and ingest-file.global_seq in all normal files with version=2 and global_seq=-1 (let rocksdb read key's real seq. If we don't set it, it will read largest seq in manifest. If we set it to other value, it will let it to be global seq, not key's real seq).

All sst files has version=2，so we don't known whether largest seq is global seq without chang global_seq in table property. We must change global_seq in table property.

But it must need a pwrite which distributed file system often not implements.

It also let rocksdb sst which be checkpointed (hard link) changed.

I think add a flag like ingest-file.version in manifest maybe make sense.

ref: https://www.facebook.com/groups/rocksdb.dev/

zxylvlp · 2019-07-23T03:47:38Z

@anand1976

siying

Thank you for working on this interesting feature. I left some comments. If you are interested, we can have a short VC call to explain my comments.

siying · 2019-07-24T23:12:11Z

db/version_edit.cc

@@ -55,6 +55,7 @@ enum CustomTag : uint32_t {
  // kMinLogNumberToKeep as part of a CustomTag as a hack. This should be
  // removed when manifest becomes forward-comptabile.
  kMinLogNumberToKeepHack = 3,
+  kHasGlobalSequence = 4,


I think this one cannot be safely ignored, so it should be 66.
The comment "Mask for an identified tag from the future which can be safely ignored." might be ambiguous.

siying · 2019-07-24T23:15:58Z

db/version_edit.cc

@@ -247,6 +254,7 @@ const char* VersionEdit::DecodeNewFile4From(Slice* input) {
  uint64_t file_size;
  SequenceNumber smallest_seqno;
  SequenceNumber largest_seqno;
+  bool has_global_seqno = false;


If we want to have a general solution, we may want a more straight-forward name for what it is. Maybe something like 'ignore_seqno_in_file' or something like that?

'largest_seqno_is_global_seqno' maybe better?

I feel 'ignore_seqno_in_file' is better than "largest_seqno_is_global_seqno".

siying · 2019-07-24T23:17:51Z

include/rocksdb/options.h

+  // table property.
+  // Warning: set it to false maybe make RepairDB useless after MANIFEST
+  // corruption.
+  bool check_cf_id_property_before_ingest = true;


Name "check" is ambiguous. We should say "allow" or not.

'allow_cf_id_property_check_before_ingest' ?

'allow_ingest_files_with_different_cf_id_property' ?

'allow_ingest_files_with_different_cf_id_property' sounds better to me.

siying · 2019-07-24T23:20:26Z

table/block_based/block.cc

+      // assert(value_type == ValueType::kTypeValue ||
+      //        value_type == ValueType::kTypeMerge ||
+      //        value_type == ValueType::kTypeDeletion ||
+      //        value_type == ValueType::kTypeRangeDeletion);


I didn't see where multiple versions of the same user key is handled. Right now I assume we generate the same key, same seqno multiple times? It might not be a good idea. What we can do here is to compare key with the previous key. If it is the same, ignore the next key.

I think db_iter & compaction_iter can handle this.

I am not sure that we handle same key same sequence number correctly. Even if we do today (I believe we don't), it's not an assumption we want to make in the future.

siying · 2019-07-24T23:24:06Z

table/block_based/block_based_table_reader.cc

@@ -1376,7 +1378,7 @@ Status BlockBasedTable::ReadPropertiesBlock(
        rep_->index_type == BlockBasedTableOptions::kBinarySearchWithFirstKey;

    s = GetGlobalSequenceNumber(*(rep_->table_properties), largest_seqno,
-                                &(rep_->global_seqno));
+                                has_global_seqno, &(rep_->global_seqno));


I didn't see anything changed in binary search dealing with seqno. Right now, binary search should still work because all keys is encoded with seqno, so when we seek to (user_key, seqno), we are guaranteed to see all entries with user_key. It's not the case if seqno is not always 0. Something needs to be done here to make it work.

OK, I would change it.

siying · 2019-07-25T17:12:29Z

db/external_sst_file_test.cc

+  s = test::DestroyDir(env_, test::TmpDir(env_) + "/external_live_db");
+  ASSERT_OK(s);
+}
+


The fact that the unit test doesn't cover the critical cases I mentioned worries me. Multiple versions of the same key, multiple merge operands of the same key, snapshot lower than seqno in the file, etc are all important cases prone to failure, and it should all be covered.

If you feel handling all the cases are too complicated, we can step back and chat about how to do things step by step.

siying · 2019-07-25T18:06:31Z

@zxylvlp also, please update the summary of the pull request.

ajkr · 2020-01-18T19:33:37Z

Is work on this ongoing? If not, I am interested in taking this and trying to move it forward.

ajkr · 2020-01-19T01:40:51Z

table/block_based/block_based_table_reader.cc

                               SequenceNumber* seqno) {
  const auto& props = table_properties.user_collected_properties;
  const auto version_pos = props.find(ExternalSstFilePropertyNames::kVersion);
  const auto seqno_pos = props.find(ExternalSstFilePropertyNames::kGlobalSeqno);

-  *seqno = kDisableGlobalSequenceNumber;
+  *seqno = has_global_seqno ? largest_seqno : kDisableGlobalSequenceNumber;


Is this taking the seqno from the table in the old DB and using that as the seqno for the table in the new DB? If so that'll break the invariant that newer versions of a given user key are in newer components of the LSM. Breaking that invariant means the DB contents will appear to change when compactions happen, and that Get() will return different results from an iterator.

largest_seqno is global_seqno stored in manifest when has_global_seqno

zxylvlp · 2020-01-19T02:59:49Z

Is work on this ongoing? If not, I am interested in taking this and trying to move it forward.

I already used this feature in our production. But I didn't track this MR recently. I am appreciate to cooperate to accomplish this MR.

facebook-github-bot added the CLA Signed label Jul 21, 2019

zxylvlp force-pushed the master branch 2 times, most recently from 71148cb to ae44e53 Compare July 21, 2019 07:56

IngestFile: support ingest SST files generated by live DB

2ecbab6

zxylvlp force-pushed the master branch from ae44e53 to 2ecbab6 Compare July 21, 2019 08:51

siying reviewed Jul 24, 2019

View reviewed changes

siying reviewed Jul 25, 2019

View reviewed changes

siying self-assigned this Sep 10, 2019

ajkr reviewed Jan 19, 2020

View reviewed changes

fredia mentioned this pull request Jul 1, 2021

IngestExternalFile() #1745

Closed

mayuehappy mentioned this pull request Mar 27, 2023

Quickly merge and Split db #11317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IngestFile: support ingest SST files generated by live DB #5602

IngestFile: support ingest SST files generated by live DB #5602

zxylvlp commented Jul 21, 2019

anand1976 commented Jul 22, 2019

zxylvlp commented Jul 23, 2019 •

edited

zxylvlp commented Jul 23, 2019

siying left a comment

siying Jul 24, 2019

zxylvlp Jul 25, 2019

siying Jul 24, 2019

zxylvlp Jul 25, 2019

siying Jul 25, 2019

siying Jul 24, 2019

zxylvlp Jul 25, 2019

zxylvlp Jul 25, 2019

siying Jul 25, 2019

siying Jul 24, 2019

zxylvlp Jul 25, 2019

siying Jul 25, 2019

siying Jul 24, 2019

zxylvlp Jul 25, 2019

siying Jul 25, 2019

siying commented Jul 25, 2019

ajkr commented Jan 18, 2020

ajkr Jan 19, 2020 •

edited

zxylvlp Jan 19, 2020

zxylvlp commented Jan 19, 2020

IngestFile: support ingest SST files generated by live DB #5602

Are you sure you want to change the base?

IngestFile: support ingest SST files generated by live DB #5602

Conversation

zxylvlp commented Jul 21, 2019

anand1976 commented Jul 22, 2019

zxylvlp commented Jul 23, 2019 • edited

zxylvlp commented Jul 23, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying commented Jul 25, 2019

ajkr commented Jan 18, 2020

ajkr Jan 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxylvlp commented Jan 19, 2020

zxylvlp commented Jul 23, 2019 •

edited

ajkr Jan 19, 2020 •

edited