Deprecate write_global_seqno and default to false (#11179)

Summary:
This option has long been intended to be set to false by default and deprecated. It might never be practical to completely remove the feature, so that we can continue to test for backward compatibility by keeping the ability to generate DBs in the old way.

Also improved API comments.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11179

Test Plan: existing tests (with one tiny update)

Reviewed By: hx235

Differential Revision: D42973927

Pulled By: pdillinger

fbshipit-source-id: e9bc161cb933266e094aea2dff8cc03753c39dab

HISTORY.md

@@ -22,6 +22,7 @@
 
 ### Public API Changes
 * Completely removed the following deprecated/obsolete statistics: the tickers `BLOCK_CACHE_INDEX_BYTES_EVICT`, `BLOCK_CACHE_FILTER_BYTES_EVICT`, `BLOOM_FILTER_MICROS`, `NO_FILE_CLOSES`, `STALL_L0_SLOWDOWN_MICROS`, `STALL_MEMTABLE_COMPACTION_MICROS`, `STALL_L0_NUM_FILES_MICROS`, `RATE_LIMIT_DELAY_MILLIS`, `NO_ITERATORS`, `NUMBER_FILTERED_DELETES`, `WRITE_TIMEDOUT`, `BLOB_DB_GC_NUM_KEYS_OVERWRITTEN`, `BLOB_DB_GC_NUM_KEYS_EXPIRED`, `BLOB_DB_GC_BYTES_OVERWRITTEN`, `BLOB_DB_GC_BYTES_EXPIRED`, `BLOCK_CACHE_COMPRESSION_DICT_BYTES_EVICT` as well as the histograms `STALL_L0_SLOWDOWN_COUNT`, `STALL_MEMTABLE_COMPACTION_COUNT`, `STALL_L0_NUM_FILES_COUNT`, `HARD_RATE_LIMIT_DELAY_COUNT`, `SOFT_RATE_LIMIT_DELAY_COUNT`, `BLOB_DB_GC_MICROS`, and `NUM_DATA_BLOCKS_READ_PER_LEVEL`. Note that as a result, the C++ enum values of the still supported statistics have changed. Developers are advised to not rely on the actual numeric values.
+* Deprecated IngestExternalFileOptions::write_global_seqno and change default to false. This option only needs to be set to true to generate a DB compatible with RocksDB versions before 5.16.0.
 
 ## 7.10.0 (01/23/2023)
 ### Behavior changes

db/external_sst_file_basic_test.cc

@@ -1182,6 +1182,7 @@ TEST_F(ExternalSSTFileBasicTest, SyncFailure) {
     ASSERT_OK(sst_file_writer->Finish());
 
     IngestExternalFileOptions ingest_opt;
+    ASSERT_FALSE(ingest_opt.write_global_seqno);  // new default
     if (i == 0) {
       ingest_opt.move_files = true;
     }

include/rocksdb/options.h

@@ -1923,7 +1923,8 @@ struct IngestExternalFileOptions {
   bool snapshot_consistency = true;
   // If set to false, IngestExternalFile() will fail if the file key range
   // overlaps with existing keys or tombstones or output of ongoing compaction
-  // during file ingestion in the DB.
+  // during file ingestion in the DB (the conditions under which a global_seqno
+  // must be assigned to the ingested file).
   bool allow_global_seqno = true;
   // If set to false and the file key range overlaps with the memtable key range
   // (memtable flush required), IngestExternalFile will fail.
@@ -1936,18 +1937,14 @@ struct IngestExternalFileOptions {
   // with allow_ingest_behind=true since the dawn of time.
   // 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
+  // DEPRECATED - Set to true if you would like to write global_seqno to
-  // the external SST file for backward compatibility. Older versions of
+  // the external SST file on ingestion for backward compatibility before
-  // RocksDB writes a global_seqno to a given offset within ingested SST files,
+  // RocksDB 5.16.0. Such old versions of RocksDB expect any global_seqno to
-  // and new versions of RocksDB do not. If you ingest an external SST using
+  // be written to the SST file rather than recorded in the DB manifest.
-  // new version of RocksDB and would like to be able to downgrade to an
+  // This functionality was deprecated because (a) random writes might be
-  // older version of RocksDB, you should set 'write_global_seqno' to true. If
+  // costly or unsupported on some FileSystems, and (b) the file checksum
-  // your service is just starting to use the new RocksDB, we recommend that
+  // changes with such a write.
-  // you set this option to false, which brings two benefits:
+  bool write_global_seqno = false;
-  // 1. No extra random write for global_seqno during ingestion.
-  // 2. Without writing external SST file, it's possible to do checksum.
-  // We have a plan to set this option to false by default in the future.
-  bool write_global_seqno = true;
   // Set to true if you would like to verify the checksums of each block of the
   // external SST file before ingestion.
   // Warning: setting this to true causes slowdown in file ingestion because

java/src/test/java/org/rocksdb/IngestExternalFileOptionsTest.java

@@ -99,9 +99,9 @@ public class IngestExternalFileOptionsTest {
   public void writeGlobalSeqno() {
     try (final IngestExternalFileOptions options =
              new IngestExternalFileOptions()) {
-      assertThat(options.writeGlobalSeqno()).isTrue();
-      options.setWriteGlobalSeqno(false);
       assertThat(options.writeGlobalSeqno()).isFalse();
+      options.setWriteGlobalSeqno(true);
+      assertThat(options.writeGlobalSeqno()).isTrue();
     }
   }
 }