Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move advanced column family options to advanced_options.h #1847

Closed
wants to merge 6 commits into from
Closed

Move advanced column family options to advanced_options.h #1847

wants to merge 6 commits into from

Conversation

IslamAbdelRahman
Copy link
Contributor

@IslamAbdelRahman IslamAbdelRahman commented Feb 7, 2017 •
edited
Loading

For the sake of making our options simpler, we should keep options.h as simple as possible and move more advanced/less common options to advaned_options.h

I started with ColumnFamilyOptions and also did some re-ordering

I have moved all ColumnFamilyOptions to advanced_options.h and only left these options in options.h

const Comparator* comparator = BytewiseComparator();
std::shared_ptr<MergeOperator> merge_operator = nullptr;
const CompactionFilter* compaction_filter = nullptr;
std::shared_ptr<CompactionFilterFactory> compaction_filter_factory = nullptr;
size_t write_buffer_size = 64 << 20;
CompressionType compression;
int level0_file_num_compaction_trigger = 4;
bool disable_auto_compactions = false;

Please feel free to comment on specific options if you think they should be advanced or should not be

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@IslamAbdelRahman
Copy link
Contributor Author

I have moved all ColumnFamilyOptions to advanced_options.h and only left these options in options.h

const Comparator* comparator = BytewiseComparator();
std::shared_ptr<MergeOperator> merge_operator = nullptr;
const CompactionFilter* compaction_filter = nullptr;
std::shared_ptr<CompactionFilterFactory> compaction_filter_factory = nullptr;
size_t write_buffer_size = 64 << 20;
CompressionType compression;
int level0_file_num_compaction_trigger = 4;
bool disable_auto_compactions = false;

Please feel free to comment on specific options if you think they should be advanced or should not be

siying
siying reviewed Feb 7, 2017
View reviewed changes
Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

include/rocksdb/options.h Outdated
// levels after base level.
//
// Default: kDisableCompressionOption (Disabled)
CompressionType bottommost_compression = kDisableCompressionOption;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should keep this one.

include/rocksdb/options.h Outdated
CompressionType bottommost_compression = kDisableCompressionOption;

// different options for compression algorithms
CompressionOptions compression_opts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this one too. If people want to tune compression, it also makes sense to tune the level.

include/rocksdb/options.h Outdated
// Default: 256MB.
//
// Dynamically changeable through SetOptions() API
uint64_t max_bytes_for_level_base = 256 * 1048576;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we say they should tune write buffer size and L0 trigger, they should tune this too accordingly. In the long term, we can make it adaptive by looking at actual L0 file size and L0 trigger. But for now, I think they have to set it.

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@IslamAbdelRahman
Copy link
Contributor Author

Move back

CompressionOptions compression_opts;
CompressionType bottommost_compression;
uint64_t max_bytes_for_level_base;

To options.h

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying
siying reviewed Feb 10, 2017
View reviewed changes
Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! My comments are to keep two more.

include/rocksdb/options.h Outdated
// 4) prefix(prefix(key)) == prefix(key)
//
// Default: nullptr
std::shared_ptr<const SliceTransform> prefix_extractor = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should keep this. This is not avoidable in many use cases.

include/rocksdb/options.h Outdated
// Default: a block-based table factory that provides a default
// implementation of TableBuilder and TableReader with default
// BlockBasedTableOptions.
std::shared_ptr<TableFactory> table_factory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block size is in it, and I think many users will need to tune the block size. block cache pointer is still in there, which is common. I would still expose it for now.

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@IslamAbdelRahman
Copy link
Contributor Author

address comments

@IslamAbdelRahman
Copy link
Contributor Author

@siying Ping!

Image of Yaktocat

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siying siying siying approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@IslamAbdelRahman @facebook-github-bot @siying