Skip to content

Commit

Permalink
Add a new SstFileWriter constructor without explicit comparator
Browse files Browse the repository at this point in the history 
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 #1978

Differential Revision: D4685629

Pulled By: sagar0

fbshipit-source-id: 372ce96
  • Loading branch information
@sagar0 @facebook-github-bot
sagar0 authored and facebook-github-bot committed Mar 13, 2017
1 parent ebd5639 commit 1ffbdfd
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 33 deletions.
6 changes: 2 additions & 4 deletions db/c.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2397,17 +2397,15 @@ 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;
}

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;
}

Expand Down
15 changes: 7 additions & 8 deletions db/external_sst_file_basic_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ class ExternalSSTFileBasicTest : public DBTestBase {
const Options options, std::vector<int> keys, int file_id,
std::map<std::string, std::string>* 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()) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -286,8 +285,8 @@ TEST_F(ExternalSSTFileBasicTest, FadviseTrigger) {
std::unique_ptr<SstFileWriter> 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)));
Expand All @@ -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)));
Expand Down
38 changes: 17 additions & 21 deletions db/external_sst_file_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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]));

Expand Down Expand Up @@ -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<std::pair<int, int>> key_ranges;
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -1636,14 +1635,12 @@ TEST_F(ExternalSSTFileTest, DirtyExit) {
std::unique_ptr<SstFileWriter> 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());
}
Expand All @@ -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";
Expand Down Expand Up @@ -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"));
Expand Down
7 changes: 7 additions & 0 deletions include/rocksdb/sst_file_writer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 1ffbdfd

Please sign in to comment.