From 0cf1008fe3b52124d6631740cd4ac9c555647ad2 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 3 Feb 2023 13:00:04 -0800 Subject: [PATCH] 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 | 1 + db/external_sst_file_basic_test.cc | 1 + include/rocksdb/options.h | 23 ++++++++----------- .../IngestExternalFileOptionsTest.java | 4 ++-- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 6b74d3ef4..fc259b92b 100644 --- a/HISTORY.md +++ b/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 diff --git a/db/external_sst_file_basic_test.cc b/db/external_sst_file_basic_test.cc index d6f3d355a..7fc5bc260 100644 --- a/db/external_sst_file_basic_test.cc +++ b/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; } diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 78e26de55..eca34108a 100644 --- a/include/rocksdb/options.h +++ b/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 - // the external SST file for backward compatibility. Older versions of - // RocksDB writes a global_seqno to a given offset within ingested SST files, - // and new versions of RocksDB do not. If you ingest an external SST using - // new version of RocksDB and would like to be able to downgrade to an - // older version of RocksDB, you should set 'write_global_seqno' to true. If - // your service is just starting to use the new RocksDB, we recommend that - // you set this option to false, which brings two benefits: - // 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; + // DEPRECATED - Set to true if you would like to write global_seqno to + // the external SST file on ingestion for backward compatibility before + // RocksDB 5.16.0. Such old versions of RocksDB expect any global_seqno to + // be written to the SST file rather than recorded in the DB manifest. + // This functionality was deprecated because (a) random writes might be + // costly or unsupported on some FileSystems, and (b) the file checksum + // changes with such a write. + bool write_global_seqno = false; // 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 diff --git a/java/src/test/java/org/rocksdb/IngestExternalFileOptionsTest.java b/java/src/test/java/org/rocksdb/IngestExternalFileOptionsTest.java index ab7e21568..230694615 100644 --- a/java/src/test/java/org/rocksdb/IngestExternalFileOptionsTest.java +++ b/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(); } } }