From dfd9725dd46c482448006d179c16066431c60e82 Mon Sep 17 00:00:00 2001
From: David Taylor <tinystatemachine@gmail.com>
Date: Mon, 11 Feb 2019 21:54:07 +0000
Subject: [PATCH] libroach: try to ingest SSTs without write-stalls

This attempts to ingest without allowing a memtable flush that could
cause write-stalls. If that fails, it then does its own, no-stall flush
and waits for it before retrying the ingest. On the re-attempt, the
ingest is allowed to do a blocking flush if it needs to, but the hope is
that the explicit flush means it will not have to.

Release note (perforrmance improvement): reduce impact of bulk data ingestion on foreground traffic with by controlling RocksDB flushes.

Release note: None
---
 c-deps/libroach/db.cc | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/c-deps/libroach/db.cc b/c-deps/libroach/db.cc
index bbb3b1e578c1..9ffb4fbc577c 100644
--- a/c-deps/libroach/db.cc
+++ b/c-deps/libroach/db.cc
@@ -697,9 +697,44 @@ DBStatus DBIngestExternalFiles(DBEngine* db, char** paths, size_t len, bool move
   ingest_options.allow_global_seqno = allow_file_modifications;
   // If there are mutations in the memtable for the keyrange covered by the file
   // being ingested, this option is checked. If true, the memtable is flushed
-  // and the ingest run. If false, an error is returned.
-  ingest_options.allow_blocking_flush = true;
+  // using a blocking, write-stalling flush and the ingest run. If false, an
+  // error is returned.
+  //
+  // We want to ingest, but we do not want a write-stall, so we initially set it
+  // to false -- if our ingest fails, we'll do a manual, no-stall flush and wait
+  // for it to finish before trying the ingest again.
+  ingest_options.allow_blocking_flush = false;
+
   rocksdb::Status status = db->rep->IngestExternalFile(paths_vec, ingest_options);
+  if (status.IsInvalidArgument()) {
+    // TODO(dt): inspect status to see if it has the message
+    //          `External file requires flush`
+    //           since the move_file and other errors also use kInvalidArgument.
+
+    // It is possible we failed because the memtable required a flush but in the
+    // options above, we set "allow_blocking_flush = false" preventing ingest
+    // from running flush with allow_write_stall = true and halting foreground
+    // traffic. Now that we know we need to flush, let's do one ourselves, with
+    // allow_write_stall = false and wait for it. After it finishes we can retry
+    // the ingest.
+    rocksdb::FlushOptions flush_options;
+    flush_options.allow_write_stall = false;
+    flush_options.wait = true;
+
+    rocksdb::Status flush_status = db->rep->Flush(flush_options);
+    if (!flush_status.ok()) {
+      return ToDBStatus(flush_status);
+    }
+
+    // Hopefully on this second attempt we will not need to flush at all, but
+    // just in case we do, we'll allow the write stall this time -- that way we
+    // can ensure we actually get the ingestion done and move on. A stalling
+    // flush is be less than ideal, but since we just flushed, a) this shouldn't
+    // happen often and b) if it does, it should be small and quick.
+    ingest_options.allow_blocking_flush = true;
+    status = db->rep->IngestExternalFile(paths_vec, ingest_options);
+  }
+
   if (!status.ok()) {
     return ToDBStatus(status);
   }