From 84a04af9a95ffbfcd120296d3507b6da8bafcda5 Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Fri, 27 Oct 2017 14:49:40 -0700 Subject: [PATCH] TableProperty::oldest_key_time defaults to 0 Summary: We don't propagate TableProperty::oldest_key_time on compaction and just write the default value to SST files. It is more natural to default the value to 0. Also revert db_sst_test back to before #2842. Closes https://github.com/facebook/rocksdb/pull/3079 Differential Revision: D6165702 Pulled By: yiwu-arbug fbshipit-source-id: ca3ce5928d96ae79a5beb12bb7d8c640a71478a0 --- db/builder.h | 6 ++---- db/db_sst_test.cc | 26 +++++++++++++++++--------- db/flush_job.cc | 1 - db/internal_stats.cc | 11 ++++++++--- include/rocksdb/table_properties.h | 5 ++--- table/block_based_table_builder.cc | 3 +-- table/block_based_table_builder.h | 2 +- table/table_builder.h | 4 +--- 8 files changed, 32 insertions(+), 26 deletions(-) diff --git a/db/builder.h b/db/builder.h index 39ec25cbc..fa96e12d2 100644 --- a/db/builder.h +++ b/db/builder.h @@ -6,7 +6,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. #pragma once -#include #include #include #include @@ -53,7 +52,7 @@ TableBuilder* NewTableBuilder( const CompressionOptions& compression_opts, int level, const std::string* compression_dict = nullptr, const bool skip_filters = false, const uint64_t creation_time = 0, - const uint64_t oldest_key_time = std::numeric_limits::max()); + const uint64_t oldest_key_time = 0); // Build a Table file from the contents of *iter. The generated file // will be named according to number specified in meta. On success, the rest of @@ -80,7 +79,6 @@ extern Status BuildTable( EventLogger* event_logger = nullptr, int job_id = 0, const Env::IOPriority io_priority = Env::IO_HIGH, TableProperties* table_properties = nullptr, int level = -1, - const uint64_t creation_time = 0, - const uint64_t oldest_key_time = std::numeric_limits::max()); + const uint64_t creation_time = 0, const uint64_t oldest_key_time = 0); } // namespace rocksdb diff --git a/db/db_sst_test.cc b/db/db_sst_test.cc index 60726d152..595668891 100644 --- a/db/db_sst_test.cc +++ b/db/db_sst_test.cc @@ -647,9 +647,18 @@ TEST_F(DBSSTTest, OpenDBWithInfiniteMaxOpenFiles) { } TEST_F(DBSSTTest, GetTotalSstFilesSize) { + // We don't propagate oldest-key-time table property on compaction and + // just write 0 as default value. This affect the exact table size, since + // we encode table properties as varint64. Force time to be 0 to work around + // it. Should remove the workaround after we propagate the property on + // compaction. + std::unique_ptr mock_env(new MockTimeEnv(Env::Default())); + mock_env->set_current_time(0); + Options options = CurrentOptions(); options.disable_auto_compactions = true; options.compression = kNoCompression; + options.env = mock_env.get(); DestroyAndReopen(options); // Generate 5 files in L0 for (int i = 0; i < 5; i++) { @@ -698,13 +707,9 @@ TEST_F(DBSSTTest, GetTotalSstFilesSize) { ASSERT_TRUE(dbfull()->GetIntProperty("rocksdb.total-sst-files-size", &total_sst_files_size)); // Live SST files = 1 (compacted file) - // The 5 bytes difference comes from oldest-key-time table property isn't - // propagated on compaction. It is written with default value - // std::numeric_limits::max as varint64. - ASSERT_EQ(live_sst_files_size, 1 * single_file_size + 5); - - // Total SST files = 5 original files + compacted file - ASSERT_EQ(total_sst_files_size, 5 * single_file_size + live_sst_files_size); + // Total SST files = 6 (5 original files + compacted file) + ASSERT_EQ(live_sst_files_size, 1 * single_file_size); + ASSERT_EQ(total_sst_files_size, 6 * single_file_size); // hold current version std::unique_ptr iter2(dbfull()->NewIterator(ReadOptions())); @@ -725,14 +730,14 @@ TEST_F(DBSSTTest, GetTotalSstFilesSize) { &total_sst_files_size)); // Live SST files = 0 // Total SST files = 6 (5 original files + compacted file) - ASSERT_EQ(total_sst_files_size, 5 * single_file_size + live_sst_files_size); + ASSERT_EQ(total_sst_files_size, 6 * single_file_size); iter1.reset(); ASSERT_TRUE(dbfull()->GetIntProperty("rocksdb.total-sst-files-size", &total_sst_files_size)); // Live SST files = 0 // Total SST files = 1 (compacted file) - ASSERT_EQ(total_sst_files_size, live_sst_files_size); + ASSERT_EQ(total_sst_files_size, 1 * single_file_size); iter2.reset(); ASSERT_TRUE(dbfull()->GetIntProperty("rocksdb.total-sst-files-size", @@ -740,6 +745,9 @@ TEST_F(DBSSTTest, GetTotalSstFilesSize) { // Live SST files = 0 // Total SST files = 0 ASSERT_EQ(total_sst_files_size, 0); + + // Close db before mock_env destruct. + Close(); } TEST_F(DBSSTTest, GetTotalSstFilesSizeVersionsFilesShared) { diff --git a/db/flush_job.cc b/db/flush_job.cc index 8faaad438..cd0af167a 100644 --- a/db/flush_job.cc +++ b/db/flush_job.cc @@ -16,7 +16,6 @@ #include #include -#include #include #include "db/builder.h" diff --git a/db/internal_stats.cc b/db/internal_stats.cc index 6196b55df..52ed4b4d9 100644 --- a/db/internal_stats.cc +++ b/db/internal_stats.cc @@ -802,10 +802,15 @@ bool InternalStats::HandleEstimateOldestKeyTime(uint64_t* value, DBImpl* /*db*/, *value = std::numeric_limits::max(); for (auto& p : collection) { *value = std::min(*value, p.second->oldest_key_time); + if (*value == 0) { + break; + } + } + if (*value > 0) { + *value = std::min({cfd_->mem()->ApproximateOldestKeyTime(), + cfd_->imm()->ApproximateOldestKeyTime(), *value}); } - *value = std::min({cfd_->mem()->ApproximateOldestKeyTime(), - cfd_->imm()->ApproximateOldestKeyTime(), *value}); - return *value < std::numeric_limits::max(); + return *value > 0 && *value < std::numeric_limits::max(); } void InternalStats::DumpDBStats(std::string* value) { diff --git a/include/rocksdb/table_properties.h b/include/rocksdb/table_properties.h index e8bbabc3b..2605fadd2 100644 --- a/include/rocksdb/table_properties.h +++ b/include/rocksdb/table_properties.h @@ -4,7 +4,6 @@ #pragma once #include -#include #include #include #include "rocksdb/status.h" @@ -164,8 +163,8 @@ struct TableProperties { // The time when the SST file was created. // Since SST files are immutable, this is equivalent to last modified time. uint64_t creation_time = 0; - // Timestamp of the earliest key - uint64_t oldest_key_time = std::numeric_limits::max(); + // Timestamp of the earliest key. 0 means unknown. + uint64_t oldest_key_time = 0; // Name of the column family with which this SST file is associated. // If column family is unknown, `column_family_name` will be an empty string. diff --git a/table/block_based_table_builder.cc b/table/block_based_table_builder.cc index 34b68c0e1..9185c607c 100644 --- a/table/block_based_table_builder.cc +++ b/table/block_based_table_builder.cc @@ -10,7 +10,6 @@ #include "table/block_based_table_builder.h" #include -#include #include #include @@ -276,7 +275,7 @@ struct BlockBasedTableBuilder::Rep { uint32_t column_family_id; const std::string& column_family_name; uint64_t creation_time = 0; - uint64_t oldest_key_time = std::numeric_limits::max(); + uint64_t oldest_key_time = 0; std::vector> table_properties_collectors; diff --git a/table/block_based_table_builder.h b/table/block_based_table_builder.h index c9197ef5c..36dfce1f0 100644 --- a/table/block_based_table_builder.h +++ b/table/block_based_table_builder.h @@ -48,7 +48,7 @@ class BlockBasedTableBuilder : public TableBuilder { const CompressionOptions& compression_opts, const std::string* compression_dict, const bool skip_filters, const std::string& column_family_name, const uint64_t creation_time = 0, - const uint64_t oldest_key_time = std::numeric_limits::max()); + const uint64_t oldest_key_time = 0); // REQUIRES: Either Finish() or Abandon() has been called. ~BlockBasedTableBuilder(); diff --git a/table/table_builder.h b/table/table_builder.h index d0ca0678e..e5e7d6e22 100644 --- a/table/table_builder.h +++ b/table/table_builder.h @@ -10,7 +10,6 @@ #pragma once #include -#include #include #include #include @@ -56,8 +55,7 @@ struct TableBuilderOptions { const CompressionOptions& _compression_opts, const std::string* _compression_dict, bool _skip_filters, const std::string& _column_family_name, int _level, - const uint64_t _creation_time = 0, - const int64_t _oldest_key_time = std::numeric_limits::max()) + const uint64_t _creation_time = 0, const int64_t _oldest_key_time = 0) : ioptions(_ioptions), internal_comparator(_internal_comparator), int_tbl_prop_collector_factories(_int_tbl_prop_collector_factories),