You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2024/08/15 05:29:49 UTC
[PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
lhotari opened a new pull request, #4480:
URL: https://github.com/apache/bookkeeper/pull/4480
Fix #4479
### Motivation
RocksDB format_version 5 has been supported since RocksDB 6.6 . It's required for certain performance optimizations.
Currently the default is format_version 2 which is really old. It's better to default to a more modern version.
### Changes
- upgrade default format_version from 2 to 5
- specify format_version=5 in various configuration files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4480:
URL: https://github.com/apache/bookkeeper/pull/4480#discussion_r1718862778
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java:
##########
@@ -198,7 +198,7 @@ private RocksDB initializeRocksDBWithBookieConf(String basePath, String subPath,
int blockSize = conf.getInt(ROCKSDB_BLOCK_SIZE, 64 * 1024);
int bloomFilterBitsPerKey = conf.getInt(ROCKSDB_BLOOM_FILTERS_BITS_PER_KEY, 10);
boolean lz4CompressionEnabled = conf.getBoolean(ROCKSDB_LZ4_COMPRESSION_ENABLED, true);
- int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 2);
+ int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 5);
Review Comment:
This code doesn't get used at all if `conf/entry_location_rocksdb.conf` or `conf/ledger_metadata_rocksdb.conf` files exist. That was changed in Bookkeeper 4.15 in https://github.com/apache/bookkeeper/pull/3056 (Pulsar 2.11, https://github.com/apache/pulsar/pull/15142).
@eolivelli Is your intention to print out the effective database version? I think it might be challenging to get it right since I believe that the given format version will only be used for new databases. For existing databases, the format is whatever it was at creation time. Just wondering if the log message would cause confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #4480:
URL: https://github.com/apache/bookkeeper/pull/4480
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #4480:
URL: https://github.com/apache/bookkeeper/pull/4480#discussion_r1718626124
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java:
##########
@@ -198,7 +198,7 @@ private RocksDB initializeRocksDBWithBookieConf(String basePath, String subPath,
int blockSize = conf.getInt(ROCKSDB_BLOCK_SIZE, 64 * 1024);
int bloomFilterBitsPerKey = conf.getInt(ROCKSDB_BLOOM_FILTERS_BITS_PER_KEY, 10);
boolean lz4CompressionEnabled = conf.getBoolean(ROCKSDB_LZ4_COMPRESSION_ENABLED, true);
- int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 2);
+ int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 5);
Review Comment:
Can we log a line at info level about this configuration ? It will be easier to troubleshot upgrades
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4480:
URL: https://github.com/apache/bookkeeper/pull/4480#discussion_r1719679373
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/KeyValueStorageRocksDB.java:
##########
@@ -198,7 +198,7 @@ private RocksDB initializeRocksDBWithBookieConf(String basePath, String subPath,
int blockSize = conf.getInt(ROCKSDB_BLOCK_SIZE, 64 * 1024);
int bloomFilterBitsPerKey = conf.getInt(ROCKSDB_BLOOM_FILTERS_BITS_PER_KEY, 10);
boolean lz4CompressionEnabled = conf.getBoolean(ROCKSDB_LZ4_COMPRESSION_ENABLED, true);
- int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 2);
+ int formatVersion = conf.getInt(ROCKSDB_FORMAT_VERSION, 5);
Review Comment:
Let's handle the possible logging separately. Bookkeeper 4.14 uses RocksDB 6.29 which supports format_version=5 (introduced in [RocksDB 6.6.0](https://github.com/facebook/rocksdb/releases/tag/v6.6.3)).
Before RocksDB 6.29, version 6.16 was used. format_version=5 support has been around for a long time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Set default format_version to 5 for RocksDB databases [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4480:
URL: https://github.com/apache/bookkeeper/pull/4480#issuecomment-2290761553
@hangc0276 @dlg99 Please review this PR. I think that this is needed before #4422.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org