From 1ffbdfd9a7637b6517053842386d71df2cd00d9b Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Mon, 13 Mar 2017 11:17:19 -0700 Subject: [PATCH] Add a new SstFileWriter constructor without explicit comparator Summary: The comparator param in SstFileWriter constructor is redundant as it already exists as a field in options. So the current SstFileWriter constructor should be deprecated in favor of a new one which does not take a comparator. Note that the jni/java apis have not been touched yet. Closes https://github.com/facebook/rocksdb/pull/1978 Differential Revision: D4685629 Pulled By: sagar0 fbshipit-source-id: 372ce96 --- db/c.cc | 6 ++--- db/external_sst_file_basic_test.cc | 15 ++++++------ db/external_sst_file_test.cc | 38 +++++++++++++----------------- include/rocksdb/sst_file_writer.h | 7 ++++++ 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/db/c.cc b/db/c.cc index 47eed825df..74e360dcea 100644 --- a/db/c.cc +++ b/db/c.cc @@ -2397,8 +2397,7 @@ void rocksdb_envoptions_destroy(rocksdb_envoptions_t* opt) { delete opt; } rocksdb_sstfilewriter_t* rocksdb_sstfilewriter_create( const rocksdb_envoptions_t* env, const rocksdb_options_t* io_options) { rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t; - writer->rep = - new SstFileWriter(env->rep, io_options->rep, io_options->rep.comparator); + writer->rep = new SstFileWriter(env->rep, io_options->rep); return writer; } @@ -2406,8 +2405,7 @@ rocksdb_sstfilewriter_t* rocksdb_sstfilewriter_create_with_comparator( const rocksdb_envoptions_t* env, const rocksdb_options_t* io_options, const rocksdb_comparator_t* comparator) { rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t; - writer->rep = - new SstFileWriter(env->rep, io_options->rep, io_options->rep.comparator); + writer->rep = new SstFileWriter(env->rep, io_options->rep); return writer; } diff --git a/db/external_sst_file_basic_test.cc b/db/external_sst_file_basic_test.cc index 1734aef053..9c6e08e7a3 100644 --- a/db/external_sst_file_basic_test.cc +++ b/db/external_sst_file_basic_test.cc @@ -41,8 +41,7 @@ class ExternalSSTFileBasicTest : public DBTestBase { const Options options, std::vector keys, int file_id, std::map* true_data) { std::string file_path = sst_files_dir_ + ToString(file_id); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator, - nullptr); + SstFileWriter sst_file_writer(EnvOptions(), options); Status s = sst_file_writer.Open(file_path); if (!s.ok()) { @@ -78,7 +77,7 @@ class ExternalSSTFileBasicTest : public DBTestBase { TEST_F(ExternalSSTFileBasicTest, Basic) { Options options = CurrentOptions(); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // Current file size should be 0 after sst_file_writer init and before open a // file. @@ -121,7 +120,7 @@ TEST_F(ExternalSSTFileBasicTest, NoCopy) { Options options = CurrentOptions(); const ImmutableCFOptions ioptions(options); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // file1.sst (0 => 99) std::string file1 = sst_files_dir_ + "file1.sst"; @@ -286,8 +285,8 @@ TEST_F(ExternalSSTFileBasicTest, FadviseTrigger) { std::unique_ptr sst_file_writer; std::string sst_file_path = sst_files_dir_ + "file_fadvise_disable.sst"; - sst_file_writer.reset(new SstFileWriter(EnvOptions(), options, - options.comparator, nullptr, false)); + sst_file_writer.reset( + new SstFileWriter(EnvOptions(), options, nullptr, false)); ASSERT_OK(sst_file_writer->Open(sst_file_path)); for (int i = 0; i < kNumKeys; i++) { ASSERT_OK(sst_file_writer->Add(Key(i), Key(i))); @@ -298,8 +297,8 @@ TEST_F(ExternalSSTFileBasicTest, FadviseTrigger) { sst_file_path = sst_files_dir_ + "file_fadvise_enable.sst"; - sst_file_writer.reset(new SstFileWriter(EnvOptions(), options, - options.comparator, nullptr, true)); + sst_file_writer.reset( + new SstFileWriter(EnvOptions(), options, nullptr, true)); ASSERT_OK(sst_file_writer->Open(sst_file_path)); for (int i = 0; i < kNumKeys; i++) { ASSERT_OK(sst_file_writer->Add(Key(i), Key(i))); diff --git a/db/external_sst_file_test.cc b/db/external_sst_file_test.cc index ecd0b33a4a..4916c04d36 100644 --- a/db/external_sst_file_test.cc +++ b/db/external_sst_file_test.cc @@ -54,8 +54,7 @@ class ExternalSSTFileTest : public DBTestBase { data.resize(uniq_iter - data.begin()); } std::string file_path = sst_files_dir_ + ToString(file_id); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator, - cfh); + SstFileWriter sst_file_writer(EnvOptions(), options, cfh); Status s = sst_file_writer.Open(file_path); if (!s.ok()) { @@ -139,7 +138,7 @@ TEST_F(ExternalSSTFileTest, Basic) { do { Options options = CurrentOptions(); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // Current file size should be 0 after sst_file_writer init and before open a file. ASSERT_EQ(sst_file_writer.FileSize(), 0); @@ -376,7 +375,7 @@ TEST_F(ExternalSSTFileTest, AddList) { options.table_properties_collector_factories.emplace_back(abc_collector); options.table_properties_collector_factories.emplace_back(xyz_collector); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // file1.sst (0 => 99) std::string file1 = sst_files_dir_ + "file1.sst"; @@ -565,7 +564,7 @@ TEST_F(ExternalSSTFileTest, AddListAtomicity) { do { Options options = CurrentOptions(); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // files[0].sst (0 => 99) // files[1].sst (100 => 199) @@ -608,7 +607,7 @@ TEST_F(ExternalSSTFileTest, AddListAtomicity) { // This situation may result in deleting the file while it's being added. TEST_F(ExternalSSTFileTest, PurgeObsoleteFilesBug) { Options options = CurrentOptions(); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // file1.sst (0 => 500) std::string sst_file_path = sst_files_dir_ + "file1.sst"; @@ -653,7 +652,7 @@ TEST_F(ExternalSSTFileTest, PurgeObsoleteFilesBug) { TEST_F(ExternalSSTFileTest, SkipSnapshot) { Options options = CurrentOptions(); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // file1.sst (0 => 99) std::string file1 = sst_files_dir_ + "file1.sst"; @@ -744,7 +743,7 @@ TEST_F(ExternalSSTFileTest, MultiThreaded) { int range_start = file_idx * keys_per_file; int range_end = range_start + keys_per_file; - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); ASSERT_OK(sst_file_writer.Open(file_names[file_idx])); @@ -842,7 +841,7 @@ TEST_F(ExternalSSTFileTest, OverlappingRanges) { Options options = CurrentOptions(); DestroyAndReopen(options); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); printf("Option config = %d\n", option_config_); std::vector> key_ranges; @@ -1252,7 +1251,7 @@ TEST_F(ExternalSSTFileTest, AddExternalSstFileWithCustomCompartor) { options.comparator = ReverseBytewiseComparator(); DestroyAndReopen(options); - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); // Generate files with these key ranges // {14 -> 0} @@ -1354,7 +1353,7 @@ TEST_F(ExternalSSTFileTest, SstFileWriterNonSharedKeys) { Options options = CurrentOptions(); DestroyAndReopen(options); std::string file_path = sst_files_dir_ + "/not_shared"; - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); std::string suffix(100, 'X'); ASSERT_OK(sst_file_writer.Open(file_path)); @@ -1636,14 +1635,12 @@ TEST_F(ExternalSSTFileTest, DirtyExit) { std::unique_ptr sst_file_writer; // Destruct SstFileWriter without calling Finish() - sst_file_writer.reset( - new SstFileWriter(EnvOptions(), options, options.comparator)); + sst_file_writer.reset(new SstFileWriter(EnvOptions(), options)); ASSERT_OK(sst_file_writer->Open(file_path)); sst_file_writer.reset(); // Destruct SstFileWriter with a failing Finish - sst_file_writer.reset( - new SstFileWriter(EnvOptions(), options, options.comparator)); + sst_file_writer.reset(new SstFileWriter(EnvOptions(), options)); ASSERT_OK(sst_file_writer->Open(file_path)); ASSERT_NOK(sst_file_writer->Finish()); } @@ -1652,11 +1649,10 @@ TEST_F(ExternalSSTFileTest, FileWithCFInfo) { Options options = CurrentOptions(); CreateAndReopenWithCF({"koko", "toto"}, options); - SstFileWriter sfw_default(EnvOptions(), options, options.comparator, - handles_[0]); - SstFileWriter sfw_cf1(EnvOptions(), options, options.comparator, handles_[1]); - SstFileWriter sfw_cf2(EnvOptions(), options, options.comparator, handles_[2]); - SstFileWriter sfw_unknown(EnvOptions(), options, options.comparator); + SstFileWriter sfw_default(EnvOptions(), options, handles_[0]); + SstFileWriter sfw_cf1(EnvOptions(), options, handles_[1]); + SstFileWriter sfw_cf2(EnvOptions(), options, handles_[2]); + SstFileWriter sfw_unknown(EnvOptions(), options); // default_cf.sst const std::string cf_default_sst = sst_files_dir_ + "/default_cf.sst"; @@ -1774,7 +1770,7 @@ TEST_F(ExternalSSTFileTest, SnapshotInconsistencyBug) { // Overwrite all keys using IngestExternalFile std::string sst_file_path = sst_files_dir_ + "file1.sst"; - SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); + SstFileWriter sst_file_writer(EnvOptions(), options); ASSERT_OK(sst_file_writer.Open(sst_file_path)); for (int i = 0; i < kNumKeys; i++) { ASSERT_OK(sst_file_writer.Add(Key(i), Key(i) + "_V2")); diff --git a/include/rocksdb/sst_file_writer.h b/include/rocksdb/sst_file_writer.h index e416a371d5..f3ed867478 100644 --- a/include/rocksdb/sst_file_writer.h +++ b/include/rocksdb/sst_file_writer.h @@ -51,6 +51,13 @@ class SstFileWriter { // the column_family is unknown. // If invalidate_page_cache is set to true, SstFileWriter will give the OS a // hint that this file pages is not needed everytime we write 1MB to the file + SstFileWriter(const EnvOptions& env_options, const Options& options, + ColumnFamilyHandle* column_family = nullptr, + bool invalidate_page_cache = true) + : SstFileWriter(env_options, options, options.comparator, column_family, + invalidate_page_cache) {} + + // Deprecated API SstFileWriter(const EnvOptions& env_options, const Options& options, const Comparator* user_comparator, ColumnFamilyHandle* column_family = nullptr,