address comments

fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

@@ -3766,6 +3766,10 @@ TEST_CASE("noSim/ShardedRocksDB/CheckpointBasic") {
 
 TEST_CASE("noSim/ShardedRocksDB/RocksDBSstFileWriter") {
 	state std::string localFile = "rocksdb-sst-file-dump.sst";
+	state std::unique_ptr<IRocksDBSstFileWriter> sstWriter = newRocksDBSstFileWriter();
+	// Write nothing to sst file
+	sstWriter->open(localFile);
+	sstWriter->finish();
 	// Write kvs1 to sst file
 	state std::map<Key, Value> kvs1({ { "a"_sr, "1"_sr },
 	                                  { "ab"_sr, "12"_sr },
@@ -3777,7 +3781,6 @@ TEST_CASE("noSim/ShardedRocksDB/RocksDBSstFileWriter") {
 	                                  { "e"_sr, "5"_sr },
 	                                  { "h"_sr, "8"_sr },
 	                                  { "ha"_sr, "81"_sr } });
-	state std::unique_ptr<IRocksDBSstFileWriter> sstWriter = newRocksDBSstFileWriter();
 	sstWriter->open(localFile);
 	for (const auto& [key, value] : kvs1) {
 		sstWriter->write(key, value);
@@ -3815,6 +3818,7 @@ TEST_CASE("noSim/ShardedRocksDB/RocksDBSstFileWriter") {
 		sstWriter->write(key, value);
 	}
 	sstWriter->finish();
+	std::cout << "second done\n";
 	// Check: sst only contains kv of kvs3
 	rocksdb::Status status;
 	rocksdb::IngestExternalFileOptions ingestOptions;

fdbserver/RocksDBCheckpointUtils.actor.cpp

@@ -548,7 +548,7 @@ ACTOR Future<Void> RocksDBCheckpointReader::doClose(RocksDBCheckpointReader* sel
 class RocksDBSstFileWriter : public IRocksDBSstFileWriter {
 public:
 	RocksDBSstFileWriter()
-	  : writer(std::make_shared<rocksdb::SstFileWriter>(rocksdb::EnvOptions(), rocksdb::Options())), hasData(false){};
+	  : writer(std::make_unique<rocksdb::SstFileWriter>(rocksdb::EnvOptions(), rocksdb::Options())), hasData(false){};
 
 	void open(const std::string localFile) override;
 
@@ -557,51 +557,47 @@ public:
 	void finish() override;
 
 private:
-	std::shared_ptr<rocksdb::SstFileWriter> writer;
+	std::unique_ptr<rocksdb::SstFileWriter> writer;
 	std::string localFile;
 	bool hasData;
 };
 
 void RocksDBSstFileWriter::open(const std::string localFile) {
-	rocksdb::Status status;
 	this->localFile = localFile;
-	status = this->writer->Open(this->localFile);
+	rocksdb::Status status = this->writer->Open(this->localFile);
 	if (!status.ok()) {
-		Error e = statusToError(status);
 		TraceEvent(SevError, "RocksDBSstFileWriterWrapperOpenFileError")
 		    .detail("LocalFile", this->localFile)
 		    .detail("Status", status.ToString());
-		throw e;
+		throw failed_to_create_checkpoint_shard_metadata();
 	}
 }
 
 void RocksDBSstFileWriter::write(const KeyRef key, const ValueRef value) {
-	rocksdb::Status status;
+	rocksdb::Status status = this->writer->Put(toSlice(key), toSlice(value));
-	status = this->writer->Put(toSlice(key), toSlice(value));
 	if (!status.ok()) {
-		Error e = statusToError(status);
 		TraceEvent(SevError, "RocksDBSstFileWriterWrapperWriteError")
 		    .detail("LocalFile", this->localFile)
 		    .detail("Key", key)
 		    .detail("Value", value)
 		    .detail("Status", status.ToString());
-		throw e;
+		throw failed_to_create_checkpoint_shard_metadata();
 	}
 	this->hasData = true;
 }
 
 void RocksDBSstFileWriter::finish() {
 	if (!this->hasData) {
+		// writer->finish() cannot create sst file with no entries
+		// So, we have to check whether any data set to be written to sst file before writer->finish()
 		return;
 	}
-	rocksdb::Status status;
+	rocksdb::Status status = this->writer->Finish();
-	status = this->writer->Finish();
 	if (!status.ok()) {
-		Error e = statusToError(status);
 		TraceEvent(SevError, "RocksDBSstFileWriterWrapperCloseError")
 		    .detail("LocalFile", this->localFile)
 		    .detail("Status", status.ToString());
-		throw e;
+		throw failed_to_create_checkpoint_shard_metadata();
 	}
 }
 

flow/include/flow/error_definitions.h

@@ -217,6 +217,7 @@ ERROR( invalid_checkpoint_format, 2044, "Invalid checkpoint format" )
 ERROR( invalid_throttle_quota_value, 2045, "Invalid quota value. Note that reserved_throughput cannot exceed total_throughput" )
 ERROR( failed_to_create_checkpoint, 2046, "Failed to create a checkpoint" )
 ERROR( failed_to_restore_checkpoint, 2047, "Failed to restore a checkpoint" )
+ERROR( failed_to_create_checkpoint_shard_metadata, 2048, "Failed to dump shard metadata for a checkpoint to a sst file")
 
 ERROR( incompatible_protocol_version, 2100, "Incompatible protocol version" )
 ERROR( transaction_too_large, 2101, "Transaction exceeds byte limit" )