Remove random writes from SST file ingestion by riversand963 · Pull Request #4172 · facebook/rocksdb

riversand963 · 2018-07-23T23:49:19Z

RocksDB used to store global_seqno in external SST files written by
SstFileWriter. During file ingestion, RocksDB uses pwrite to update the
global_seqno. Since random write is not supported in some non-POSIX compliant
file systems, external SST file ingestion is not supported on these file
systems. To address this limitation, we no longer update global_seqno during
file ingestion. Later RocksDB uses the MANIFEST and other information in table
properties to deduce global seqno for externally-ingested SST files.

Test plan:

$make clean && make -j16 all check
$./external_sst_file_test
$./external_sst_file_basic_test

RocksDB used to store global_seqno in external SST files written by SstFileWriter. During file ingestion, RocksDB uses `pwrite` to update the `global_seqno`. Since random write is not supported in some non-POSIX compliant file systems, external SST file ingestion is not supported on these file systems. To address this limitation, we no longer update `global_seqno` during file ingestion. Later RocksDB uses the MANIFEST and other information in table properties to deduce global seqno for externally-ingested SST files. Test plan: ``` $make clean && make -j16 all check $./external_sst_file_test $./external_sst_file_basic_test ```

facebook-github-bot

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

riversand963 · 2018-07-23T23:54:07Z

@siying Could you PTAL? Thanks

siying

Is there a way we write a special flag in ExternalSstFilePropertyNames::kGlobalSeqno to indicate that this is not a valid seqno? Or maybe bump up "rocksdb.external_sst_file.version" and remove "rocksdb.external_sst_file.global_seqno" as a whole.

siying · 2018-07-24T17:22:00Z

db/version_edit.h

-  FileDescriptor(uint64_t number, uint32_t path_id, uint64_t _file_size)
+  FileDescriptor(uint64_t number, uint32_t path_id, uint64_t _file_size,
+                 SequenceNumber _smallest_seqno = kMaxSequenceNumber,
+                 SequenceNumber _largest_seqno = 0)


How hard it is to avoid defaults?

siying · 2018-07-24T17:31:16Z

table/block_based_table_reader.cc

  }

-  SequenceNumber global_seqno = DecodeFixed64(seqno_pos->second.c_str());
+  SequenceNumber global_seqno = largest_seqno;


Can we still read from DecodeFixed64(seqno_pos->second.c_str()) and make sure it is either the same as largest_seqno or it is not modified?

by 'not modified', do you mean verify the checksum?

…wr_2

facebook-github-bot · 2018-07-24T21:33:57Z

@riversand963 has updated the pull request. Re-import the pull request

riversand963 · 2018-07-25T18:26:57Z

Closing this PR, moving to #4188 to avoid unrelated file changes.

…wr_2

facebook-github-bot · 2018-07-25T23:45:35Z

@riversand963 has updated the pull request. Re-import the pull request

riversand963 · 2018-07-25T23:45:52Z

@siying can you PTAL?

facebook-github-bot · 2018-07-26T00:02:12Z

@riversand963 has updated the pull request. Re-import the pull request

siying

Good job!

siying · 2018-07-26T18:06:36Z

include/rocksdb/options.h

  // All files will be ingested at the bottommost level with seqno=0.
  bool ingest_behind = false;
+  // Set to true if you would like to write global_seqno to a given offset in
+  // the external SST file.


Mention the backward compatibility scenario that this option is tries to address and recommend to set it false to new services or the compatibility is not needed.

siying · 2018-07-26T19:08:36Z

table/block_based_table_reader.cc

  }

  SequenceNumber global_seqno = DecodeFixed64(seqno_pos->second.c_str());
+  assert(global_seqno == 0 || global_seqno == largest_seqno);


Rather than assert, I suggest explicitly fail the open and return corrupted

siying · 2018-07-26T19:09:30Z

table/table_builder.h

                     const InternalKeyComparator& _internal_comparator,
                     bool _skip_filters = false, bool _immortal = false,
-                     int _level = -1)
+                     int _level = -1, SequenceNumber _largest_seqno = 0)


Can we remove the defaults? I don't see a scenario we should do it.

Do you mean defaults of all the parameters?

I don't care level, but I don't think it makes sense to have a default for largest_seqno. I don't care whether you reorder to two so you can keep the level default, or remove defaults for both of level and largest_seqno.

Done. Adding an overloaded constructor. Another option is to provide a setter (not implemented).

siying · 2018-07-26T20:06:39Z

table/block_based_table_reader.cc

@@ -697,6 +697,8 @@ SequenceNumber GetGlobalSequenceNumber(const TableProperties& table_properties,
  }

  SequenceNumber global_seqno = DecodeFixed64(seqno_pos->second.c_str());


I'm surprised that the existing code has no verification that seqno_pos is valid.

…wr_2

facebook-github-bot · 2018-07-26T22:45:29Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

…wr_2

facebook-github-bot · 2018-07-27T17:07:35Z

@riversand963 has updated the pull request. Re-import the pull request

…wr_2

facebook-github-bot · 2018-07-27T17:10:37Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2018-07-27T17:39:06Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

…wr_2

facebook-github-bot · 2018-07-27T21:52:49Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

Summary: RocksDB used to store global_seqno in external SST files written by SstFileWriter. During file ingestion, RocksDB uses `pwrite` to update the `global_seqno`. Since random write is not supported in some non-POSIX compliant file systems, external SST file ingestion is not supported on these file systems. To address this limitation, we no longer update `global_seqno` during file ingestion. Later RocksDB uses the MANIFEST and other information in table properties to deduce global seqno for externally-ingested SST files. Pull Request resolved: facebook#4172 Differential Revision: D8961465 Pulled By: riversand963 fbshipit-source-id: 4382ec85270a96be5bc0cf33758ca2b167b05071

This uses he new flag added in facebook/rocksdb#4172 to avoid the global_seq_no edits and the SST copying they forced us to do to preserve the raft log. Passing this flag has to be gated on a cluster version that is defined well after the upgrade to rocks 5.17 -- using the flag requires that older versions of RocksDB (< 5.16) will never be used to read, as they will still be looking for and using the seq_no in the file. Release note (performance improvement): reduce write-amplification during bulk-loading (IMPORT and RESTORE)

34806: util/mon: Downgrade "bytes usage increases" log message r=knz a=bdarnell This message can be logged frequently in certain workloads, and its appearance in the info log is not actionable since there is no way to tie it back to the query that caused it. I see no reason to have these messages enabled by default. Release note: None 34886: storage/engine,libroach: 1x write amplification on ingest sst r=dt a=dt This uses the new flag added in facebook/rocksdb#4172 to avoid needing to make global_seq_no-related edits to SSTs, and thus avoid the SST copying those edits forced us to do to preserve the raft log immutability when hard-linking directly to side load SSTs. This is gated on a cluster version that is defined well after the upgrade to rocks 5.17, since using the flag requires that no older versions of RocksDB (<5.16), will ever be used to read these SSTs, as they will lack the seq_no that the older versions need for correctness. Release note (performance improvement): reduce write-amplification during bulk-loading (IMPORT and RESTORE) 34902: sql: ensure spans get anonymized in plan collection r=knz a=knz Fixes #34889. Release note (bug fix): The logical plans collected for display in the web UI now also hide the details of which key ranges are scanned in table lookups. Co-authored-by: Ben Darnell <ben@bendarnell.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

Summary: It it not safe to assume application had sync the SST file before ingest it into DB. Also the directory to put the ingested file needs to be fsync, otherwise the file can be lost. For integrity of RocksDB we need to sync the ingested file and directory before apply the change to manifest. Also syncing after writing global sequence when write_global_seqno=true was removed in #4172. Adding it back. Fixes #5287. Pull Request resolved: #5435 Test Plan: Test ingest file with ldb command and observe fsync/fdatasync in strace output. Tried both move_files=true and move_files=false. https://gist.github.com/yiwu-arbug/650a4023f57979056d83485fa863bef9 More test suggestions are welcome. Differential Revision: D15941675 Pulled By: riversand963 fbshipit-source-id: 389533f3923065a96df2cdde23ff4724a1810d78

Summary: It it not safe to assume application had sync the SST file before ingest it into DB. Also the directory to put the ingested file needs to be fsync, otherwise the file can be lost. For integrity of RocksDB we need to sync the ingested file and directory before apply the change to manifest. Also syncing after writing global sequence when write_global_seqno=true was removed in facebook#4172. Adding it back. Fixes facebook#5287. Pull Request resolved: facebook#5435 Test Plan: Test ingest file with ldb command and observe fsync/fdatasync in strace output. Tried both move_files=true and move_files=false. https://gist.github.com/yiwu-arbug/650a4023f57979056d83485fa863bef9 More test suggestions are welcome. Differential Revision: D15941675 Pulled By: riversand963 fbshipit-source-id: 389533f3923065a96df2cdde23ff4724a1810d78

riversand963 added 2 commits July 23, 2018 16:39

Fix format

f5dfae6

riversand963 requested a review from siying July 23, 2018 23:49

riversand963 self-assigned this Jul 23, 2018

facebook-github-bot added the CLA Signed label Jul 23, 2018

facebook-github-bot reviewed Jul 23, 2018

View reviewed changes

riversand963 mentioned this pull request Jul 23, 2018

[WIP] [Don't Review] Add support for non-random-write ingestion #4163

Closed

siying reviewed Jul 24, 2018

View reviewed changes

riversand963 added 2 commits July 24, 2018 12:55

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

d84b2f8

…wr_2

Add code comment for newly-added class members.

6a85a36

riversand963 changed the title ~~Remove random writes from SST file ingestion~~ [DO NOT REVIEW] Remove random writes from SST file ingestion Jul 25, 2018

riversand963 changed the title ~~[DO NOT REVIEW] Remove random writes from SST file ingestion~~ Remove random writes from SST file ingestion Jul 25, 2018

riversand963 closed this Jul 25, 2018

riversand963 reopened this Jul 25, 2018

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

7f87822

…wr_2

riversand963 mentioned this pull request Jul 25, 2018

Remove random write from SST file ingestion #4188

Closed

Address comments.

107f387

Add a new option to IngestExternalFileOptions

a7e1025

siying approved these changes Jul 26, 2018

View reviewed changes

riversand963 added 2 commits July 26, 2018 13:54

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

fcdbf14

…wr_2

Address review comments

3d99d4e

facebook-github-bot reviewed Jul 26, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

d129ad8

…wr_2

Change variable type from char[] to std::arry

55e77e0

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

d36accd

…wr_2

facebook-github-bot reviewed Jul 27, 2018

View reviewed changes

Fix a compilation error on Windows

55246c3

facebook-github-bot reviewed Jul 27, 2018

View reviewed changes

riversand963 added 2 commits July 27, 2018 14:45

Merge remote-tracking branch 'upstream/master' into ingestion_no_rand…

efad269

…wr_2

Fix format

3476cce

facebook-github-bot reviewed Jul 27, 2018

View reviewed changes

facebook-github-bot closed this in 54de568 Jul 27, 2018

riversand963 mentioned this pull request Aug 13, 2018

store global sequence number in FileMetadata #4270

Closed

petermattis mentioned this pull request Sep 4, 2018

sqlccl: AddSSTable 1x write amplification cockroachdb/cockroach#18888

Closed

dt mentioned this pull request Feb 14, 2019

storage/engine,libroach: 1x write amplification on ingest sst cockroachdb/cockroach#34886

Merged

yiwu-arbug mentioned this pull request Jun 10, 2019

Fix ingested file and direcotry not being sync #5435

Closed

		@@ -697,6 +697,8 @@ SequenceNumber GetGlobalSequenceNumber(const TableProperties& table_properties,
		}

		SequenceNumber global_seqno = DecodeFixed64(seqno_pos->second.c_str());

Conversation

riversand963 commented Jul 23, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

riversand963 commented Jul 23, 2018

Uh oh!

siying left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 24, 2018

Uh oh!

riversand963 commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jul 25, 2018

Uh oh!

riversand963 commented Jul 25, 2018

Uh oh!

facebook-github-bot commented Jul 26, 2018

Uh oh!

siying left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 26, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 27, 2018

Uh oh!

facebook-github-bot commented Jul 27, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 27, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 27, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

riversand963 commented Jul 25, 2018 •

edited

Loading