Skip to content

Commit

Permalink
Make the options in table_builder/block_builder less misleading
Browse files Browse the repository at this point in the history
Summary:
By original design, the regular `block options` and index `block options` in table_builder is mutable. We can use ChangeOptions to change the options directly.

However, with my last change, `BlockBuilder` no longer hold the reference to the index_block_options -- as a result, any changes made after the creation of index block builder will be of no effect.

But still the code is very error-prone and developers can easily fall into the trap without aware of it. To avoid this problem from happening in the future, I deleted the `ChangeOptions` and the `index_block_options`, as well as many other changes to make it less misleading.

Test Plan:
make
make check
make release

Reviewers: dhruba, haobo

Reviewed By: haobo

CC: leveldb

Differential Revision: https://reviews.facebook.net/D13707
  • Loading branch information
liukai committed Nov 17, 2013
1 parent f7a2b97 commit 7604e2f
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 11 deletions.
8 changes: 4 additions & 4 deletions table/block_based_table_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ void LogStatsCollectionError(

struct BlockBasedTableBuilder::Rep {
Options options;
Options index_block_options;
WritableFile* file;
uint64_t offset = 0;
Status status;
Expand Down Expand Up @@ -105,10 +104,11 @@ struct BlockBasedTableBuilder::Rep {

Rep(const Options& opt, WritableFile* f, CompressionType compression_type)
: options(opt),
index_block_options(opt),
file(f),
data_block(&options),
index_block(1, index_block_options.comparator),
data_block(options),
// To avoid linear scan, we make the block_restart_interval to be `1`
// in index block builder
index_block(1 /* block_restart_interval */, options.comparator),
compression_type(compression_type),
filter_block(opt.filter_policy == nullptr ? nullptr
: new FilterBlockBuilder(opt)) {
Expand Down
4 changes: 2 additions & 2 deletions table/block_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ BlockBuilder::BlockBuilder(int block_restart_interval,
restarts_.push_back(0); // First restart point is at offset 0
}

BlockBuilder::BlockBuilder(const Options* options)
: BlockBuilder(options->block_restart_interval, options->comparator) {
BlockBuilder::BlockBuilder(const Options& options)
: BlockBuilder(options.block_restart_interval, options.comparator) {
}

void BlockBuilder::Reset() {
Expand Down
4 changes: 2 additions & 2 deletions table/block_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class Comparator;

class BlockBuilder {
public:
BlockBuilder(int block_restart_interval, const Comparator* comparator);
explicit BlockBuilder(const Options* options);
BlockBuilder(int block_builder, const Comparator* comparator);
explicit BlockBuilder(const Options& options);

// Reset the contents as if the BlockBuilder was just constructed.
void Reset();
Expand Down
2 changes: 1 addition & 1 deletion table/block_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ TEST(BlockTest, SimpleTest) {
Options options = Options();
std::vector<std::string> keys;
std::vector<std::string> values;
BlockBuilder builder(&options);
BlockBuilder builder(options);
int num_records = 100000;
char buf[10];
char* p = &buf[0];
Expand Down
4 changes: 2 additions & 2 deletions table/table_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class BlockConstructor: public Constructor {
virtual Status FinishImpl(const Options& options, const KVMap& data) {
delete block_;
block_ = nullptr;
BlockBuilder builder(&options);
BlockBuilder builder(options);

for (KVMap::const_iterator it = data.begin();
it != data.end();
Expand Down Expand Up @@ -832,7 +832,7 @@ TEST(TableTest, BasicTableStats) {
ASSERT_EQ("", stats.filter_policy_name); // no filter policy is used

// Verify data size.
BlockBuilder block_builder(&options);
BlockBuilder block_builder(options);
for (const auto& item : kvmap) {
block_builder.Add(item.first, item.second);
}
Expand Down

0 comments on commit 7604e2f

Please sign in to comment.