Skip to content

Conversation

@dlg99
Copy link
Contributor

@dlg99 dlg99 commented Jun 6, 2024

Descriptions of the changes in this PR:

Fix #4409

Motivation

Upgrade RocksDB to v.9.2.1 to pick up latest performance and stability improvements and fixes in deleteRanges.
Hopefully deleteRanges() is stable enough for us to try again perf improvements #3653 that were reverted previously.

Changes

Changed RocksDB version, updated code to use changed API.

@dlg99 dlg99 requested review from hangc0276 and hezhangjian June 7, 2024 15:32
Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Have you verified any compatible issues from 6.16.x to 9.2.1 and 7.x to 9.2.1?

@dlg99
Copy link
Contributor Author

dlg99 commented Jun 9, 2024

Have you verified any compatible issues from 6.16.x to 9.2.1 and 7.x to 9.2.1?

@hangc0276 I fixed/updated backward compat tests, that's it. If these aren't enough we need to invest some time into these kinds of tests

@dlg99
Copy link
Contributor Author

dlg99 commented Jun 18, 2024

@hangc0276 I rebased after the merge of downgrade compat tests; upgrade to 9.2.1 doesn't downgrade nicely. I am not yet sure what can be configured to support compatibility.

@lhotari
Copy link
Member

lhotari commented Aug 15, 2024

@hangc0276 I rebased after the merge of downgrade compat tests; upgrade to 9.2.1 doesn't downgrade nicely. I am not yet sure what can be configured to support compatibility.

In RocksDB 9.0.0 release notes there's "format_version=6 is the new default setting in BlockBasedTableOptions, for more robust data integrity checking. DBs and SST files written with this setting cannot be read by RocksDB versions before 8.6.0."

@dlg99 Perhaps setting dbStorage_rocksDB_format_version=5 instead of dbStorage_rocksDB_format_version=2 could help solve this issue? It seems that format_version=2 is really old and doesn't support many optimizations.

@lhotari
Copy link
Member

lhotari commented Aug 15, 2024

I created an issue #4479 about upgrading the default format_version to 5.

@lhotari
Copy link
Member

lhotari commented Aug 15, 2024

I created PR #4480 to upgrade default format_version to 5.

@hangc0276
Copy link
Contributor

@lhotari Have you tested the downgrade? We set the format_version to 2 by default, and it may not impacted by version 6

@lhotari
Copy link
Member

lhotari commented Aug 19, 2024

@lhotari Have you tested the downgrade? We set the format_version to 2 by default, and it may not impacted by version 6

@hangc0276 The format_version setting was missing for conf/ledger_metadata_rocksdb.conf. that's been a problem previously. I haven't tested it yet.

@hangc0276
Copy link
Contributor

@lhotari Have you tested the downgrade? We set the format_version to 2 by default, and it may not impacted by version 6

@hangc0276 The format_version setting was missing for conf/ledger_metadata_rocksdb.conf. that's been a problem previously. I haven't tested it yet.

@lhotari It is set to 2

@lhotari
Copy link
Member

lhotari commented Aug 19, 2024

@lhotari It is set to 2

@hangc0276 Yes, you are right that format_version should already be set to 2.
I was assuming that the ledger metadata database would have been the problem. However, when conf/entry_location_rocksdb.conf is missing, the format_version set in code would get used.
(logic

if (dbConfigType == DbConfigType.EntryLocation) {
dbFilePath = conf.getEntryLocationRocksdbConf();
} else if (dbConfigType == DbConfigType.LedgerMetadata) {
dbFilePath = conf.getLedgerMetadataRocksdbConf();
} else {
dbFilePath = conf.getDefaultRocksDBConf();
}
log.info("Searching for a RocksDB configuration file in {}", dbFilePath);
if (Paths.get(dbFilePath).toFile().exists()) {
log.info("Found a RocksDB configuration file and using it to initialize the RocksDB");
db = initializeRocksDBWithConfFile(basePath, subPath, dbConfigType, conf, readOnly, dbFilePath);
} else {
log.info("Haven't found the file and read the configuration from the main bookkeeper configuration");
db = initializeRocksDBWithBookieConf(basePath, subPath, dbConfigType, conf, readOnly);
}
)
By default in BK, the file is missing since the provide file is conf/entry_location_rocksdb.conf.default and that has no impact.

However, since format_version 2 is ancient, you never know if it starts causing problems when continuing to use it with RocskDB 9.x+. I haven't yet investigated the problem.

@yurivict
Copy link

yurivict commented Oct 6, 2024

RocksDB is now at 9.6.1.

Can this PR be updated to 9.6.1 and merged?

@StevenLuMT
Copy link
Member

PRs that have not been processed for a long time will be closed first. Please open them yourself if necessary.

@lhotari
Copy link
Member

lhotari commented Mar 5, 2025

@hangc0276 I rebased after the merge of downgrade compat tests; upgrade to 9.2.1 doesn't downgrade nicely. I am not yet sure what can be configured to support compatibility.

@dlg99 It seems that format version wasn't specified for all databases in Bookkeeper. #4559 will address that for the cases where configuration isn't read from the configuration files. In master branch, reading from configuration files has been broken since #4407 and that's fixed in #4560. After these are merged, it would make sense to resume this PR. Are you interested in continuing this work? Perhaps we could bump directly to latest 9.x of RocksDB, 9.10.0

@hangc0276
Copy link
Contributor

@hangc0276 I rebased after the merge of downgrade compat tests; upgrade to 9.2.1 doesn't downgrade nicely. I am not yet sure what can be configured to support compatibility.

@dlg99 It seems that format version wasn't specified for all databases in Bookkeeper. #4559 will address that for the cases where configuration isn't read from the configuration files. In master branch, reading from configuration files has been broken since #4407 and that's fixed in #4560. After these are merged, it would make sense to resume this PR. Are you interested in continuing this work? Perhaps we could bump directly to latest 9.x of RocksDB, 9.10.0

@lhotari We should hold on to the upgrade before verifying the compatibility issue.
#4559 set formatVersion to 5. Does it mean all the old BookKeeper versions are need to upgrade to a version that contains #4559, and then upgrade to the version that upgraded RocksDB to 9.x?

@lhotari
Copy link
Member

lhotari commented Mar 22, 2025

@lhotari We should hold on to the upgrade before verifying the compatibility issue.
#4559 set formatVersion to 5. Does it mean all the old BookKeeper versions are need to upgrade to a version that contains #4559, and then upgrade to the version that upgraded RocksDB to 9.x?

@hangc0276 No, it doesn't mean that. formatVersion 5 has been around for a long time. The problem was that not all databases had the formatVersion set and that caused the problem in downgrading, since format 6 is the default since 8.8 (IIRC) unless explicitly set.

@lhotari
Copy link
Member

lhotari commented Apr 16, 2025

Superseded by #4580

@lhotari lhotari closed this Apr 16, 2025
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.

Task: upgrade RocksDB

6 participants