[IngestExternalFile] Allow ingesting overlapping files #5539

igorcanadi · 2019-07-04T13:57:38Z

Currently IngestExternalFile() fails when its input files' ranges overlap. This condition doesn't need to hold for files that are to be ingested in L0, though.

This commit allows overlapping files and forces their target level to L0.

Additionally, ingest job's completion is logged to EventLogger, analogous to flush and compaction jobs.

riversand963

Thanks @igorcanadi for the PR. The change itself mostly LGTM except two questions.
On the one hand, allowing multiple files to be ingested to L0 can potentially cause write stalls to occur faster due to the number of L0 increasing. Can you advise on how to properly use the new interface?
On the other hand, if the files have overlapping keys, will it be possible to ingest them one-by-one? Or you want them to have the same seqno?

riversand963 · 2019-07-15T22:13:08Z

db/external_sst_file_ingestion_job.cc

@@ -250,6 +255,13 @@ void ExternalSstFileIngestionJob::UpdateStats() {
  uint64_t total_keys = 0;
  uint64_t total_l0_files = 0;
  uint64_t total_time = env_->NowMicros() - job_start_time_;
+
+  auto stream = event_logger_->Log();


Maybe specify the type of stream explicitly? We prefer to use auto when the type is easily known from the context.

riversand963 · 2019-07-15T22:36:02Z

The Travis test ExternalSSTFileTest.AddList also fails.

igorcanadi · 2019-07-23T22:33:36Z

On the one hand, allowing multiple files to be ingested to L0 can potentially cause write stalls to occur faster due to the number of L0 increasing. Can you advise on how to properly use the new interface?

In my case I'm using the interface in a bulk load scenario, where compactions are disabled until the very end, when we execute one giant compaction. I agree it is tricky to ingest files to level 0, but I don't think that's the reason to not allow it.

On the other hand, if the files have overlapping keys, will it be possible to ingest them one-by-one? Or you want them to have the same seqno?

It would be possible to ingest them one-by-one. However, I'm using this in the context of RocksDB-Cloud (https://github.com/rockset/rocksdb-cloud), where each write to MANIFEST requires one write to cloud storage (S3). As such, adding N files one-by-one adds significant single-threaded latency and slows down my ingest. In my case I do not care about the seqno.

The Travis test ExternalSSTFileTest.AddList also fails.

Will take a look.

igorcanadi · 2019-07-23T23:22:22Z

@riversand963 I fixed the tests and addressed your comment.

riversand963 · 2019-07-26T18:08:47Z

Hi @igorcanadi . I chatted with @siying offline. We are concerned about one issue.
If we allow ingestion of overlapping key ranges to level0, two internal keys in two SSTs may be exactly the same. This is due to current implementation of file ingestion. It is preferable to make different versions of the same user key have different seqno in order to minimize confusion. I understand that your use case is not interested in seqno, but are you willing to handle this in your PR?

igorcanadi · 2019-07-26T18:17:44Z

@riversand963 That makes sense. I have to admit I'm not familiar with how we assign seqnos to ingested files -- Would you mind shortly explaining what would be the best way to address the issue?

petermattis · 2019-07-26T18:19:01Z

Hi @igorcanadi . I chatted with @siying offline. We are concerned about one issue.
If we allow ingestion of overlapping key ranges to level0, two internal keys in two SSTs may be exactly the same. This is due to current implementation of file ingestion. It is preferable to make different versions of the same user key have different seqno in order to minimize confusion. I understand that your use case is not interested in seqno, but are you willing to handle this in your PR?

Right now there is a single sequence number assigned to all tables, but it wouldn't be difficult to give each ingested sstable a different global sequence number.

siying · 2019-07-26T18:25:20Z

@igorcanadi the related logic is here: https://github.com/facebook/rocksdb/blob/6.3fb/db/external_sst_file_ingestion_job.cc#L215-L244

igorcanadi · 2019-07-29T23:14:13Z

@siying @riversand963 I updated the commit to assign different sequence numbers to different files when the ranges overlap.

ajkr · 2019-08-12T18:25:25Z

Will this be merged? We found that even ingesting non-overlapping files to L0 in a single IngestExternalFile() causes an assertion failure (test case is https://gist.github.com/ajkr/2874c8348ff4731cd10a3bea505d720d). One of the invariants on L0 files is the seqnum of a newer L0 file has to be strictly greater than the seqnums of an older L0 file. So if multiple files are ingested to L0 with the same seqnum, the invariant is violated. I'm sure there are other ways to fix this besides this PR (like relaxing the invariant), but this PR could also solve the problem.

ajkr · 2019-09-03T23:00:18Z

I think this is a good change that fixes two issues (atomically ingesting multiple overlapping files to a DB and atomically ingesting multiple non-overlapping files to L0). Is anybody available to review?

siying · 2019-09-10T00:46:22Z

@riversand963 do you remember the status to this PR?

riversand963 · 2019-09-12T22:51:23Z

@igorcanadi sorry for the delay in code review. Would you mind rebasing this PR to latest master? Overall LGTM. Will take another look and land if everything looks good.

Currently IngestExternalFile() fails when its input files' ranges overlap. This condition doesn't need to hold for files that are to be ingested in L0, though. This commit allows overlapping files and forces their target level to L0. Additionally, ingest job's completion is logged to EventLogger, analogous to flush and compaction jobs.

igorcanadi · 2019-09-13T12:53:22Z

@riversand963 Rebased.

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 · 2019-09-13T19:28:26Z

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

riversand963

LGTM. I also adjusted format and made two small changes.
Thanks @igorcanadi for the contribution!

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 · 2019-09-13T22:09:55Z

@riversand963 merged this pull request in 9763135.

This is necessary to revert cockroachdb/cockroach#39562 to support RocksDB built without `-DNDEBUG`. Consecutive files ingested to L0 may be assigned the same seqnum, and this PR updates the LSM validation to allow that. There is a longer-term fix in facebook#5539. We should take that later, maybe after the 19.2 release. Test Plan: ran some tests that previously failed with corruption error like `ENABLE_ROCKSDB_ASSERTIONS=1 make PKG=github.com/cockroachdb/cockroach/pkg/ccl/backupccl testrace`; verified they succeed now.

Summary: Currently IngestExternalFile() fails when its input files' ranges overlap. This condition doesn't need to hold for files that are to be ingested in L0, though. This commit allows overlapping files and forces their target level to L0. Additionally, ingest job's completion is logged to EventLogger, analogous to flush and compaction jobs. Pull Request resolved: facebook#5539 Differential Revision: D17370660 Pulled By: riversand963 fbshipit-source-id: 749a3899b17d1be267a5afd5b0a99d96b38ab2f3

fredzqm · 2021-03-24T20:11:30Z

db/db_impl/db_impl.cc

+        assert(!!consumed_seqno_count ==
+               !!ingestion_jobs[i].ConsumedSequenceNumbersCount());
+        consumed_seqno_count +=
+            ingestion_jobs[i].ConsumedSequenceNumbersCount();


This line makes debug build and production build different. Is this a bug?

consumed_seqno_count += ingestion_jobs[i].ConsumedSequenceNumbersCount();

We managed to hit the assertion here in tests due to some but not all sstable file overlap with db.

assert(!!consumed_seqno_count == !!ingestion_jobs[i].ConsumedSequenceNumbersCount());

What is the !! supposed to do? That is a new operator to me...

I think !! force converts a integer to a boolean. 0 => 0, others => 1

I also posted the question here: https://groups.google.com/g/rocksdb/c/g5LACzCltWI
Thanks in advance!

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

riversand963 reviewed Jul 15, 2019

View reviewed changes

igorcanadi force-pushed the allow_overlapping_ingest branch from 05df5e6 to 243cef3 Compare July 23, 2019 23:21

igorcanadi force-pushed the allow_overlapping_ingest branch from 243cef3 to 947fef2 Compare July 29, 2019 23:12

ajkr mentioned this pull request Aug 13, 2019

storage: fix and re-enable rocksdb assertions cockroachdb/cockroach#39564

Closed

siying assigned riversand963 Sep 10, 2019

igorcanadi force-pushed the allow_overlapping_ingest branch from 947fef2 to 6cf74b0 Compare September 13, 2019 12:53

facebook-github-bot reviewed Sep 13, 2019

View reviewed changes

riversand963 added 3 commits September 13, 2019 12:00

Run make format

8105768

Move outer arg after in-arg

638ee8c

Reset files_overlap_ flag in cleanup upon failure

f3a2a27

riversand963 approved these changes Sep 13, 2019

View reviewed changes

facebook-github-bot reviewed Sep 13, 2019

View reviewed changes

facebook-github-bot closed this in 9763135 Sep 13, 2019

facebook-github-bot added the Merged label Sep 13, 2019

ajkr mentioned this pull request Sep 26, 2019

relax L0 seqno invariant for ingested files cockroachdb/rocksdb#59

Merged

tong-hao mentioned this pull request Nov 26, 2019

Ingest Failed: Not implemented: Files have overlapping ranges vesoft-inc/nebula#1337

Closed

fredzqm reviewed Mar 24, 2021

View reviewed changes

riversand963 mentioned this pull request Oct 11, 2021

Fix sequence number bump logic in multi-CF SST ingestion #9005

Closed

3pointer mentioned this pull request Jan 4, 2024

raftstore: batch ingest ssts if there is no pending writes during apply tikv/tikv#16286

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IngestExternalFile] Allow ingesting overlapping files #5539

[IngestExternalFile] Allow ingesting overlapping files #5539

igorcanadi commented Jul 4, 2019

riversand963 left a comment

riversand963 Jul 15, 2019

riversand963 commented Jul 15, 2019

igorcanadi commented Jul 23, 2019

igorcanadi commented Jul 23, 2019

riversand963 commented Jul 26, 2019

igorcanadi commented Jul 26, 2019

petermattis commented Jul 26, 2019

siying commented Jul 26, 2019

igorcanadi commented Jul 29, 2019

ajkr commented Aug 12, 2019

ajkr commented Sep 3, 2019 •

edited

Loading

siying commented Sep 10, 2019

riversand963 commented Sep 12, 2019

igorcanadi commented Sep 13, 2019

facebook-github-bot left a comment

facebook-github-bot commented Sep 13, 2019

riversand963 left a comment

facebook-github-bot left a comment

facebook-github-bot commented Sep 13, 2019

fredzqm Mar 24, 2021 •

edited

Loading

mrambacher Mar 24, 2021

fredzqm Mar 24, 2021

fredzqm Mar 24, 2021

[IngestExternalFile] Allow ingesting overlapping files #5539

[IngestExternalFile] Allow ingesting overlapping files #5539

Conversation

igorcanadi commented Jul 4, 2019

riversand963 left a comment

Choose a reason for hiding this comment

riversand963 Jul 15, 2019

Choose a reason for hiding this comment

riversand963 commented Jul 15, 2019

igorcanadi commented Jul 23, 2019

igorcanadi commented Jul 23, 2019

riversand963 commented Jul 26, 2019

igorcanadi commented Jul 26, 2019

petermattis commented Jul 26, 2019

siying commented Jul 26, 2019

igorcanadi commented Jul 29, 2019

ajkr commented Aug 12, 2019

ajkr commented Sep 3, 2019 • edited Loading

siying commented Sep 10, 2019

riversand963 commented Sep 12, 2019

igorcanadi commented Sep 13, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 13, 2019

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 13, 2019

fredzqm Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

mrambacher Mar 24, 2021

Choose a reason for hiding this comment

fredzqm Mar 24, 2021

Choose a reason for hiding this comment

fredzqm Mar 24, 2021

Choose a reason for hiding this comment

ajkr commented Sep 3, 2019 •

edited

Loading

fredzqm Mar 24, 2021 •

edited

Loading