Skip to content

Commit

Permalink
Disable secondary test with sst truncation deletion; API clarification (
Browse files Browse the repository at this point in the history
#13395)

Summary:
**Context/Summary:**
Secondary DB relies on open file descriptor of the shared SST file in primary DB to continue being able to read the file even if that file is deleted in the primary DB. However, this won't work if the file is truncated instead of deleted, which triggers an "truncated block read" corruption in stress test on secondary db reads. Truncation can happen if RocksDB implementation of SSTFileManager and `bytes_max_delete_chunk>0` are used. This PR is to disable such testing combination in stress test and clarify the related API.

Pull Request resolved: #13395

Test Plan:
- Manually repro-ed with below UT. I'm in favor of not including this UT in the codebase as it should be self-evident from the API comment now about the incompatiblity. Secondary DB is in a direction of being replaced by Follower so we should minimize edge-case tests for code with no functional change for a to-be-replaced functionality.
```
TEST_F(DBSecondaryTest, IncompatibleWithPrimarySSTTruncation) {
  Options options;
  options.env = env_;
  options.disable_auto_compactions = true;
  options.sst_file_manager.reset(NewSstFileManager(
      env_, nullptr /*fs*/, "" /*trash_dir*/, 2024000 /*rate_bytes_per_sec*/,
      true /*delete_existing_trash*/, nullptr /*status*/,
      0.25 /*max_trash_db_ratio*/, 1129 /*bytes_max_delete_chunk*/));
  Reopen(options);

  ASSERT_OK(Put("key1", "old_value"));
  ASSERT_OK(Put("key2", "old_value"));
  ASSERT_OK(Flush());
  ASSERT_OK(Put("key1", "new_value"));
  ASSERT_OK(Put("key3", "new_value"));
  ASSERT_OK(Flush());

  Options options1;
  options1.env = env_;
  options1.max_open_files = -1;
  Reopen(options);
  OpenSecondary(options1);
  ASSERT_OK(db_secondary_->TryCatchUpWithPrimary());

  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
      "DeleteScheduler::DeleteTrashFile:Fsync", [&](void*) {
        std::string value;
        Status s = db_secondary_->Get(ReadOptions(), "key2", &value);
        assert(s.IsCorruption());
        assert(s.ToString().find("truncated block read") !=
            std::string::npos);
      });
  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();

  ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr));

  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks();
}
```
- Monitor future stress test

Reviewed By: jowlyzhang

Differential Revision: D69499694

Pulled By: hx235

fbshipit-source-id: 57525b9841897f42aecb758a4d3dd3589367dcd9
  • Loading branch information
hx235 authored and facebook-github-bot committed Feb 12, 2025
1 parent 8234d67 commit f6b2cdd
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
7 changes: 7 additions & 0 deletions include/rocksdb/db.h
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,13 @@ class DB {
// delete it after use.
//
// Return OK on success, non-OK on failures.
//
// WARNING: Secondary databases cannot read shared SST files that have been
// truncated in the primary database. To avoid compatibility issues, users
// should refrain from using features in the primary database that can cause
// truncation, such as setting `rate_bytes_per_sec > 0` and
// `bytes_max_delete_chunk > 0` when invoking RocksDB's implementation of
// `NewSstFileManager()`.
static Status OpenAsSecondary(const Options& options, const std::string& name,
const std::string& secondary_path,
std::unique_ptr<DB>* dbptr);
Expand Down
8 changes: 7 additions & 1 deletion tools/db_crashtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,13 @@ def finalize_and_sanitize(src_params):
if dest_params.get("atomic_flush", 0) == 1:
# disable pipelined write when atomic flush is used.
dest_params["enable_pipelined_write"] = 0
if dest_params.get("sst_file_manager_bytes_per_sec", 0) == 0:
# Truncating SST files in primary DB is incompatible
# with secondary DB since the latter can't read the shared
# and truncated SST file correctly
if (
dest_params.get("sst_file_manager_bytes_per_sec", 0) == 0
or dest_params.get("test_secondary") == 1
):
dest_params["sst_file_manager_bytes_per_truncate"] = 0
if dest_params.get("prefix_size") == -1:
dest_params["readpercent"] += dest_params.get("prefixpercent", 20)
Expand Down

0 comments on commit f6b2cdd

Please sign in to comment.