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

HDDS-11404. Make RocksDB all parameters configurable. #7150

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

weimingdiit
Copy link
Contributor

@weimingdiit weimingdiit commented Sep 3, 2024

What changes were proposed in this pull request?

Make RocksDB all parameters configurable.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11404

How was this patch tested?

unit tests and local test environment, and i will provide some test results from my local test environment

Additional notes:

  1. We can use different ini files to customize the rocksDB configuration of corresponding services (such as OM, SCM, DN).
    For example, you can create a file such as om.db.ini to configure OM's rocksDB, scm.db.ini to configure SCM's rocksDB, and container.db.ini to configure DN's rocksDB
  2. OM, SCM, and DN were tested separately, and the results were in line with expectations

for OM

om.db.ini file:

image

OPTION file:

image
image
image
image

for SCM

scm.db.ini file:

image

OPTION file:

image
image
image
image
image

for DN

container.db.ini file:

image

OPTION file:

image
image
image
image
image
image
image

@weimingdiit
Copy link
Contributor Author

@jojochuang @szetszwo @adoroszlai Could you help review this PR? Thanks.

@weimingdiit weimingdiit force-pushed the HDDS-11404 branch 3 times, most recently from 1e4113a to 44a1ed1 Compare September 3, 2024 12:37
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@weimingdiit , just have a quick comment inlined.

pom.xml Outdated
Comment on lines 1534 to 1544
<allowedImport>org.rocksdb.AccessHint</allowedImport>
<allowedImport>org.rocksdb.BuiltinComparator</allowedImport>
<allowedImport>org.rocksdb.ChecksumType</allowedImport>
<allowedImport>org.rocksdb.CompactionPriority</allowedImport>
<allowedImport>org.rocksdb.CompactionStopStyle</allowedImport>
<allowedImport>org.rocksdb.CompressionType</allowedImport>
<allowedImport>org.rocksdb.DataBlockIndexType</allowedImport>
<allowedImport>org.rocksdb.IndexShorteningMode</allowedImport>
<allowedImport>org.rocksdb.IndexType</allowedImport>
<allowedImport>org.rocksdb.PrepopulateBlobCache</allowedImport>
<allowedImport>org.rocksdb.WALRecoveryMode</allowedImport>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move the new classes to hadoop-hdds/managed-rocksdb/... instead of adding allowedImport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szetszwo Thanks for your comments, I have updated the code to fully support configuring all rocksDB parameters via ini files

@kerneltime
Copy link
Contributor

@weimingdiit instead of wrapping every config via Ozone, why not let Ozone specify the RockDB config file https://github.com/facebook/rocksdb/blob/5b40c0c074f16dae6e1d60a3c5d7aa3e7d63fdc7/include/rocksdb/utilities/options_util.h#L56
This way, we do not need to keep managing RocksDB Config value additions or deprecation.

@errose28
Copy link
Contributor

errose28 commented Sep 3, 2024

@kerneltime I agree. I made the same suggestion in #7117 with a pointer to the RocksDB docs. The approach taken here is not sustainable to maintain and also less intuitive than having Ozone work with RocksDB's config file exactly how RocksDB documents it.

This DBProfile abstraction is old and not really used as far as I know. It might be better to just get rid of it. We can instead directly provide example RocksDB config files for different use cases. This will be much easier to follow than hiding the tuning inside the Java code.

@weimingdiit
Copy link
Contributor Author

@errose28 @kerneltime This is a very good idea. I looked at the relevant code and felt that directly supporting INI files might be a better choice. Thank you very much for your suggestion. I will modify the code to fully support INI files.
By the way, I think this issue #7117 should be closed

@sumitagrawl
Copy link
Contributor

@weimingdiit Do we have mechanism to control service specific usecase, like if OM, SCM and DN have different configuration, like WAL manual flush may need disable only for OM ?

@sadanand48
Copy link
Contributor

Do we have mechanism to control service specific usecase, like if OM, SCM and DN have different configuration, like WAL manual flush may need disable only for OM ?

Probably we could have configuration files for each Role . By default all of these can point to a base config but also provide an option to add overrides for each role. Similar to ozone.metadata.dirs where each process can define its own metadata dir but if not defined , we can fallback to the common config

@weimingdiit
Copy link
Contributor Author

Do we have mechanism to control service specific usecase, like if OM, SCM and DN have different configuration, like WAL manual flush may need disable only for OM ?

Probably we could have configuration files for each Role . By default all of these can point to a base config but also provide an option to add overrides for each role. Similar to ozone.metadata.dirs where each process can define its own metadata dir but if not defined , we can fallback to the common config

@sumitagrawl @sadanand48 Yes, this may be our ultimate goal.

@weimingdiit
Copy link
Contributor Author

weimingdiit commented Sep 16, 2024

@errose28 @szetszwo Hi, could you help review the pr?

@weimingdiit weimingdiit marked this pull request as draft October 28, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants