andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add a new SstFileWriter constructor without explicit comparator

Browse Source 
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
This commit is contained in:
Sagar Vemuri 2017-03-13 11:17:19 -07:00 committed by Facebook Github Bot
parent ebd5639b6d
commit 1ffbdfd9a7
4 changed files with 33 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
db/c.cc
View File

@ -2397,8 +2397,7 @@ void rocksdb_envoptions_destroy(rocksdb_envoptions_t* opt) { delete opt; }
rocksdb_sstfilewriter_t* rocksdb_sstfilewriter_create( rocksdb_sstfilewriter_t* rocksdb_sstfilewriter_create(
const rocksdb_envoptions_t* env, const rocksdb_options_t* io_options) { const rocksdb_envoptions_t* env, const rocksdb_options_t* io_options) {
rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t; rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t;
writer->rep = writer->rep = new SstFileWriter(env->rep, io_options->rep);
new SstFileWriter(env->rep, io_options->rep, io_options->rep.comparator);
return writer; 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_envoptions_t* env, const rocksdb_options_t* io_options,
const rocksdb_comparator_t* comparator) { const rocksdb_comparator_t* comparator) {
rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t; rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t;
writer->rep = writer->rep = new SstFileWriter(env->rep, io_options->rep);
new SstFileWriter(env->rep, io_options->rep, io_options->rep.comparator);
return writer; return writer;
} }

15
db/external_sst_file_basic_test.cc
View File

@ -41,8 +41,7 @@ class ExternalSSTFileBasicTest : public DBTestBase {
const Options options, std::vector<int> keys, int file_id, const Options options, std::vector<int> keys, int file_id,
std::map<std::string, std::string>* true_data) { std::map<std::string, std::string>* true_data) {
std::string file_path = sst_files_dir_ + ToString(file_id); std::string file_path = sst_files_dir_ + ToString(file_id);
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator, SstFileWriter sst_file_writer(EnvOptions(), options);
nullptr);
Status s = sst_file_writer.Open(file_path); Status s = sst_file_writer.Open(file_path);
if (!s.ok()) { if (!s.ok()) {
@ -78,7 +77,7 @@ class ExternalSSTFileBasicTest : public DBTestBase {
TEST_F(ExternalSSTFileBasicTest, Basic) { TEST_F(ExternalSSTFileBasicTest, Basic) {
Options options = CurrentOptions(); 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 // Current file size should be 0 after sst_file_writer init and before open a
// file. // file.
@ -121,7 +120,7 @@ TEST_F(ExternalSSTFileBasicTest, NoCopy) {
Options options = CurrentOptions(); Options options = CurrentOptions();
const ImmutableCFOptions ioptions(options); const ImmutableCFOptions ioptions(options);
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
// file1.sst (0 => 99) // file1.sst (0 => 99)
std::string file1 = sst_files_dir_ + "file1.sst"; std::string file1 = sst_files_dir_ + "file1.sst";
@ -286,8 +285,8 @@ TEST_F(ExternalSSTFileBasicTest, FadviseTrigger) {
std::unique_ptr<SstFileWriter> sst_file_writer; std::unique_ptr<SstFileWriter> sst_file_writer;
std::string sst_file_path = sst_files_dir_ + "file_fadvise_disable.sst"; std::string sst_file_path = sst_files_dir_ + "file_fadvise_disable.sst";
sst_file_writer.reset(new SstFileWriter(EnvOptions(), options, sst_file_writer.reset(
options.comparator, nullptr, false)); new SstFileWriter(EnvOptions(), options, nullptr, false));
ASSERT_OK(sst_file_writer->Open(sst_file_path)); ASSERT_OK(sst_file_writer->Open(sst_file_path));
for (int i = 0; i < kNumKeys; i++) { for (int i = 0; i < kNumKeys; i++) {
ASSERT_OK(sst_file_writer->Add(Key(i), Key(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_path = sst_files_dir_ + "file_fadvise_enable.sst";
sst_file_writer.reset(new SstFileWriter(EnvOptions(), options, sst_file_writer.reset(
options.comparator, nullptr, true)); new SstFileWriter(EnvOptions(), options, nullptr, true));
ASSERT_OK(sst_file_writer->Open(sst_file_path)); ASSERT_OK(sst_file_writer->Open(sst_file_path));
for (int i = 0; i < kNumKeys; i++) { for (int i = 0; i < kNumKeys; i++) {
ASSERT_OK(sst_file_writer->Add(Key(i), Key(i))); ASSERT_OK(sst_file_writer->Add(Key(i), Key(i)));

38
db/external_sst_file_test.cc
View File

@ -54,8 +54,7 @@ class ExternalSSTFileTest : public DBTestBase {
data.resize(uniq_iter - data.begin()); data.resize(uniq_iter - data.begin());
} }
std::string file_path = sst_files_dir_ + ToString(file_id); std::string file_path = sst_files_dir_ + ToString(file_id);
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator, SstFileWriter sst_file_writer(EnvOptions(), options, cfh);
cfh);
Status s = sst_file_writer.Open(file_path); Status s = sst_file_writer.Open(file_path);
if (!s.ok()) { if (!s.ok()) {
@ -139,7 +138,7 @@ TEST_F(ExternalSSTFileTest, Basic) {
do { do {
Options options = CurrentOptions(); 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. // Current file size should be 0 after sst_file_writer init and before open a file.
ASSERT_EQ(sst_file_writer.FileSize(), 0); 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(abc_collector);
options.table_properties_collector_factories.emplace_back(xyz_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) // file1.sst (0 => 99)
std::string file1 = sst_files_dir_ + "file1.sst"; std::string file1 = sst_files_dir_ + "file1.sst";
@ -565,7 +564,7 @@ TEST_F(ExternalSSTFileTest, AddListAtomicity) {
do { do {
Options options = CurrentOptions(); Options options = CurrentOptions();
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
// files[0].sst (0 => 99) // files[0].sst (0 => 99)
// files[1].sst (100 => 199) // 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. // This situation may result in deleting the file while it's being added.
TEST_F(ExternalSSTFileTest, PurgeObsoleteFilesBug) { TEST_F(ExternalSSTFileTest, PurgeObsoleteFilesBug) {
Options options = CurrentOptions(); Options options = CurrentOptions();
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
// file1.sst (0 => 500) // file1.sst (0 => 500)
std::string sst_file_path = sst_files_dir_ + "file1.sst"; std::string sst_file_path = sst_files_dir_ + "file1.sst";
@ -653,7 +652,7 @@ TEST_F(ExternalSSTFileTest, PurgeObsoleteFilesBug) {
TEST_F(ExternalSSTFileTest, SkipSnapshot) { TEST_F(ExternalSSTFileTest, SkipSnapshot) {
Options options = CurrentOptions(); Options options = CurrentOptions();
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
// file1.sst (0 => 99) // file1.sst (0 => 99)
std::string file1 = sst_files_dir_ + "file1.sst"; 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_start = file_idx * keys_per_file;
int range_end = range_start + 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])); ASSERT_OK(sst_file_writer.Open(file_names[file_idx]));
@ -842,7 +841,7 @@ TEST_F(ExternalSSTFileTest, OverlappingRanges) {
Options options = CurrentOptions(); Options options = CurrentOptions();
DestroyAndReopen(options); DestroyAndReopen(options);
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
printf("Option config = %d\n", option_config_); printf("Option config = %d\n", option_config_);
std::vector<std::pair<int, int>> key_ranges; std::vector<std::pair<int, int>> key_ranges;
@ -1252,7 +1251,7 @@ TEST_F(ExternalSSTFileTest, AddExternalSstFileWithCustomCompartor) {
options.comparator = ReverseBytewiseComparator(); options.comparator = ReverseBytewiseComparator();
DestroyAndReopen(options); DestroyAndReopen(options);
SstFileWriter sst_file_writer(EnvOptions(), options, options.comparator); SstFileWriter sst_file_writer(EnvOptions(), options);
// Generate files with these key ranges // Generate files with these key ranges
// {14 -> 0} // {14 -> 0}
@ -1354,7 +1353,7 @@ TEST_F(ExternalSSTFileTest, SstFileWriterNonSharedKeys) {
Options options = CurrentOptions(); Options options = CurrentOptions();
DestroyAndReopen(options); DestroyAndReopen(options);
std::string file_path = sst_files_dir_ + "/not_shared"; 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'); std::string suffix(100, 'X');
ASSERT_OK(sst_file_writer.Open(file_path)); ASSERT_OK(sst_file_writer.Open(file_path));
@ -1636,14 +1635,12 @@ TEST_F(ExternalSSTFileTest, DirtyExit) {
std::unique_ptr<SstFileWriter> sst_file_writer; std::unique_ptr<SstFileWriter> sst_file_writer;
// Destruct SstFileWriter without calling Finish() // Destruct SstFileWriter without calling Finish()
sst_file_writer.reset( sst_file_writer.reset(new SstFileWriter(EnvOptions(), options));
new SstFileWriter(EnvOptions(), options, options.comparator));
ASSERT_OK(sst_file_writer->Open(file_path)); ASSERT_OK(sst_file_writer->Open(file_path));
sst_file_writer.reset(); sst_file_writer.reset();
// Destruct SstFileWriter with a failing Finish // Destruct SstFileWriter with a failing Finish
sst_file_writer.reset( sst_file_writer.reset(new SstFileWriter(EnvOptions(), options));
new SstFileWriter(EnvOptions(), options, options.comparator));
ASSERT_OK(sst_file_writer->Open(file_path)); ASSERT_OK(sst_file_writer->Open(file_path));
ASSERT_NOK(sst_file_writer->Finish()); ASSERT_NOK(sst_file_writer->Finish());
} }
@ -1652,11 +1649,10 @@ TEST_F(ExternalSSTFileTest, FileWithCFInfo) {
Options options = CurrentOptions(); Options options = CurrentOptions();
CreateAndReopenWithCF({"koko", "toto"}, options); CreateAndReopenWithCF({"koko", "toto"}, options);
SstFileWriter sfw_default(EnvOptions(), options, options.comparator, SstFileWriter sfw_default(EnvOptions(), options, handles_[0]);
handles_[0]); SstFileWriter sfw_cf1(EnvOptions(), options, handles_[1]);
SstFileWriter sfw_cf1(EnvOptions(), options, options.comparator, handles_[1]); SstFileWriter sfw_cf2(EnvOptions(), options, handles_[2]);
SstFileWriter sfw_cf2(EnvOptions(), options, options.comparator, handles_[2]); SstFileWriter sfw_unknown(EnvOptions(), options);
SstFileWriter sfw_unknown(EnvOptions(), options, options.comparator);
// default_cf.sst // default_cf.sst
const std::string cf_default_sst = sst_files_dir_ + "/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 // Overwrite all keys using IngestExternalFile
std::string sst_file_path = sst_files_dir_ + "file1.sst"; 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)); ASSERT_OK(sst_file_writer.Open(sst_file_path));
for (int i = 0; i < kNumKeys; i++) { for (int i = 0; i < kNumKeys; i++) {
ASSERT_OK(sst_file_writer.Add(Key(i), Key(i) + "_V2")); ASSERT_OK(sst_file_writer.Add(Key(i), Key(i) + "_V2"));

7
include/rocksdb/sst_file_writer.h
View File

@ -51,6 +51,13 @@ class SstFileWriter {
// the column_family is unknown. // the column_family is unknown.
// If invalidate_page_cache is set to true, SstFileWriter will give the OS a // 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 // 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, SstFileWriter(const EnvOptions& env_options, const Options& options,
const Comparator* user_comparator, const Comparator* user_comparator,
ColumnFamilyHandle* column_family = nullptr, ColumnFamilyHandle* column_family = nullptr,