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

Feature:Online Query and Dynamic Modification of Table-level RocksDB Options #1511

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

ruojieranyishen
Copy link
Collaborator

@ruojieranyishen ruojieranyishen commented Jun 3, 2023

What problem does this PR solve?

Feature:Online Query and Dynamic Modification of Table-level RocksDB Options (#1488)

#1488

Complete the dynamic setting function of num_levels and write_buffer_size option.I use rocksdb.write_buffer_size as a dynamically modifiable parameter and rocksdb.num_levels as a non-dynamically modifiable parameter to test my idea.

What is changed and how does it work?

  1. Providing online modification functionality for Pegasus table-level RocksDB configuration:
    1. The common RocksDB options can still be configured through the config.ini file.
    2. For the RocksDB options that support dynamic modification, they can be modified using the create or set_app_envs command.
    3. For the RocksDB options that do not support dynamic modification, they can be modified using the create command.
  2. Providing online query functionality for Pegasus table-level RocksDB configuration:
    1. Following the design principle of app environment (app env), only the RocksDB options set through the create or set_app_envs command will be available for online querying.
    2. The remaining RocksDB option information is located in the [pegasus.server] section of the config.ini file.
Tests
  • Unit test
    Unit test is add in meta_app_envs_test.update_app_envs_test
  • Manual test (add detailed scripts or steps below)
    Pegasus shell case:
    create lpf -e rocksdb.num_levels=12,rocksdb.write_buffer_size=100
    create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=100
    create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=33554432
    set_app_envs rocksdb.write_buffer_size 67108864
    set_app_envs rocksdb.num_levels 4
    get_app_envs

… RocksDB Options (apache#1488)

apache#1488

Complete the dynamic setting function of num_levels and write_buffer_size option.I use rocksdb.write_buffer_size as a dynamically modifiable parameter and rocksdb.num_levels as a non-dynamically modifiable parameter to test my idea.

Pegasus shell case:
create lpf -e rocksdb.num_levels=12,rocksdb.write_buffer_size=100
create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=100
create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=33554432
set_app_envs rocksdb.write_buffer_size  67108864
set_app_envs rocksdb.num_levels 4
get_app_envs
…Options (apache#1488)

apache#1488
Currently, I only perform strict validation of app envs during the create_app command process. However, in the set_app_envs command, there is no validation for invalid envs, and they are simply not processed.

- Manual test (add detailed scripts or steps below)
Pegasus shell case:
create lpf -e rocksdb.num_levels=12,rocksdb.write_buffer_size=100
create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=100
create lpf -e rocksdb.num_levels=5,rocksdb.write_buffer_size=33554432
set_app_envs rocksdb.write_buffer_size  67108864
set_app_envs rocksdb.write_buffer_size  100
get_app_envs
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.h Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/common/replica_envs.h Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
@cr-gpt
Copy link

cr-gpt bot commented Jun 13, 2023

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

hycdong
hycdong previously approved these changes Jun 14, 2023
src/server/pegasus_server_impl.h Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/meta/app_env_validator.cpp Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
@cr-gpt
Copy link

cr-gpt bot commented Jun 19, 2023

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

src/meta/server_state.cpp Show resolved Hide resolved
src/meta/app_env_validator.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
…ode.

Mainly the following aspects:
1. Improve the unit test to ensure that the new option can also be added correctly.
2. Add getter and setter methods for option.
src/meta/test/meta_app_operation_test.cpp Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Show resolved Hide resolved
src/base/pegasus_const.cpp Outdated Show resolved Hide resolved
src/base/pegasus_const.cpp Outdated Show resolved Hide resolved
src/base/pegasus_const.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/test/pegasus_server_impl_test.cpp Outdated Show resolved Hide resolved
src/server/test/pegasus_server_impl_test.cpp Outdated Show resolved Hide resolved
src/server/test/pegasus_server_impl_test.cpp Outdated Show resolved Hide resolved
src/server/test/pegasus_server_impl_test.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/base/pegasus_const.cpp Outdated Show resolved Hide resolved
src/base/pegasus_const.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
src/server/pegasus_server_impl.cpp Outdated Show resolved Hide resolved
ROCKSDB_NUM_LEVELS,
};

const std::unordered_map<std::string, cf_opts_setter> cf_opts_setters = {
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked the functions provided by rocksdb here? I guess it's not needed to check the options ourselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I didn't see this function before. Can i solve this problem in next PR?

Copy link
Member

Choose a reason for hiding this comment

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

That's OK to me, thanks!

@acelyc111 acelyc111 merged commit fc61628 into apache:master Aug 11, 2023
73 checks passed
@empiredan empiredan mentioned this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants