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.

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

Enable prefix bloom filter by default #2860

Merged
merged 5 commits into from
Sep 25, 2021
Merged

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Sep 14, 2021

  1. Enable prefix bloom filter, close whole key bloom filter by default. (previously we use whole key bloom filter by default). Benchmark is listed here. In read/write scenario, the read performance is doubled, while pure read is not impacted. PS: the memory usage will shrink as well.
  2. We need to get some property in rocksdb during test. So add a http interface to get rocksdb property, space name and legal property need to be provided. (No need to set enable_rocksdb_statistics to true). For example:
curl -G "http://127.0.0.1:52005/rocksdb_property?space=ldbc_snb_sf10&property=rocksdb.levelstats"

Close #2601

@randomJoe211, please check if we need to add a doc about the http interfaces.
@HarrisChu, I have check the compatibility during test, everything works fine. Please pay attention to compatible test later. (write new data to old storage, check data)

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Sep 14, 2021
@whitewum whitewum added the doc affected PR: improvements or additions to documentation label Sep 14, 2021
Comment on lines +1186 to +1187
auto eng = folly::stringPrintf("Engine %zu", i);
obj[eng] = std::move(value(val));
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@darionyaphet
Copy link
Contributor

only display on HTTP interface ?

@critical27
Copy link
Contributor Author

only display on HTTP interface ?

Yeah, rocksdb stats also provide in http only

Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

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

LGTM. do we submit this PR to 2.6?

@critical27
Copy link
Contributor Author

LGTM. do we submit this PR to 2.6?

Yeah, I have tested before. The compatibility need to check carefully later.

@critical27 critical27 linked an issue Sep 23, 2021 that may be closed by this pull request
Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

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

Good job

}
bbtOpts.whole_key_filtering = FLAGS_enable_rocksdb_whole_key_filtering;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a question:
When enable_rocksdb_prefix_filtering and enable_rocksdb_whole_key_filterin are both true,
which one is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of them could used at the same time.

rocksdb::ReadOptions options;
// prefix_same_as_start is false by default
options.total_order_seek = FLAGS_enable_rocksdb_prefix_filtering;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@critical27 critical27 merged commit 5fe655a into vesoft-inc:master Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QE]optimization of rate-limit [STO]Bloom filter (Currently we use full-key)
6 participants