HDDS-14225. Upgrade RocksDB from 7.7.3 to 10.10.1#9813
Conversation
Generated-by: GPT-5.3-Codex
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @smengcl for working on this.
We should detect OS at runtime, not build time. Single build (on any OS) should create binaries for all supported operating systems.
-
dependencyManagementin root POM should include an entry forrocksdbjniartifact with each supported classifier, as well as the non-classified artifact:<dependency> <groupId>org.rocksdb</groupId> <artifactId>rocksdbjni</artifactId> <version>${rocksdb.version}</version> </dependency> <dependency> <groupId>org.rocksdb</groupId> <artifactId>rocksdbjni</artifactId> <version>${rocksdb.version}</version> <classifier>linux64</classifier> </dependency> <dependency> <groupId>org.rocksdb</groupId> <artifactId>rocksdbjni</artifactId> <version>${rocksdb.version}</version> <classifier>osx</classifier> </dependency> ...
-
hdds-rocks-nativeshould unpack all of these -
other modules should continue depending on platform-independent
rocksdbjni(no classifier)
Thus changes in most pom.xml files are not needed, nor is rocksdbjni.classifier.
This is a workaround for RocksDB 10.4.2 thin-jar packaging. Ensure classifier JNI artifacts are present at runtime/tests and keep dependency analysis stable. Not needed if mvnrepo rocksdbjni provides a fat jar containing native libs for all supported platforms.
…or change RocksDB 9.10.0 changed DB::KeyMayExist behavior semantics to follow its function comment. Ozone snapshot code paths were treating keyMayExist/getIfExist misses as definitive, which could misclassify existing keys in snapshot-related flows under RocksDB >= 9.10.0 and cause test failures. Treat keyMayExist/getIfExist as hints and fall back to point reads before deciding not-found: - RDBTable: verify with get()/get(ByteBuffer) on inconclusive keyMayExist - TypedTable: in codec-buffer isExist path, fallback to full get before returning false - KeyManagerImpl and ReclaimableRenameEntryFilter: use getSkipCache fallback for snapshot rename lookups RocksDB changelog for reference: https://github.com/facebook/rocksdb/releases/tag/v9.10.0 Behavior Changes DB::KeyMayExist() now follows its function comment, which means value parameter can be null, and it will be set only if value_found is passed in.
RocksDB 9.10+ may consume ByteBuffer state during keyMayExist; treat it as a hint and always use duplicated key buffers for keyMayExist/get fallback paths in RDBTable/RocksDatabase. Add a regression unit test for ByteBuffer fallback behavior. Also make replicas-test.sh restore the whole container.db directory (not just one file) to avoid stale RocksDB WAL/MANIFEST artifacts when recovering from backup.
431f2b2 to
c9b83ef
Compare
c9b83ef to
7270ac0
Compare
Replace restored schema-v3 container.db directory
|
Good news. rocksdb team just published 10.5.1 jars: https://repo1.maven.org/maven2/org/rocksdb/rocksdbjni/10.5.1/ But we need at least 10.9.0 to avoid the forward compatibility bug mentioned in the description. Need to wait a bit more |
fcef039 to
ed1d278
Compare
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it. |
The RocksDB 10 upgrade replaced the deterministic SST diff assertion with a column-family membership check that passes even when getSSTDiffList returns an empty list for every input, so a regression in the diff engine would go undetected. Restore strong coverage without the version-brittle hard-coded SST file names: - Derive a per-snapshot baseline diff at runtime by tracking all tables. - Assert each table subset's diff equals that baseline filtered to the subset's column families (null-CF files always kept), in order, via containsExactlyElementsOf. - Add a guard that at least one snapshot pair yields a non-empty diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Upgrades Apache Ozone’s embedded RocksDB JNI from 7.7.3 to 10.10.1.1, while aiming to preserve on-disk compatibility and behavioral semantics across upgrades/downgrades, and adapts Ozone’s RocksDB integration to API changes in RocksDB 9/10.
Changes:
- Bump RocksDB JNI version and update the native
hdds-rocks-nativetooling build (including patch updates and source-tag version derivation). - Pin SST block-based table format version to 5 in Ozone-managed configs/writers to keep SSTs readable after downgrade prior to finalization.
- Migrate away from removed
RocksDB#deleteFile, and adjustkeyMayExist/existence-check semantics and tests to match RocksDB 9.10+ contract changes.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Bumps global rocksdb.version to 10.10.1.1. |
| hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug-keywords.robot | Refactors container-state verification keywords for debug smoketests. |
| hadoop-ozone/dist/src/main/compose/common/replicas-test.sh | Improves debug-compose replica test flow, including container DB backup/restore and pipeline state cleanup. |
| hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java | Makes SST-diff/link assertions less dependent on fixed SST filenames across RocksDB versions. |
| hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/hadoop/hdds/utils/db/RDBSstFileWriter.java | Pins SST format version for SST files written via SstFileWriter. |
| hadoop-hdds/rocks-native/src/main/patches/rocks-native.patch | Updates native tools patch (eg. raw_sst_file_reader/iterator) for RocksDB 10.x API changes. |
| hadoop-hdds/rocks-native/pom.xml | Derives upstream RocksDB source tag version from rocksdb.version and aligns tarball/build paths. |
| hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java | Replaces file deletion with range-based deletion (deleteFilesInRanges) and waits for on-disk removal. |
| hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedDBOptions.java | Adjusts logger setter to match RocksDB 9+ LoggerInterface signature change while keeping leak-tracking. |
| hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedBloomFilter.java | Adds equals/hashCode overrides to satisfy SpotBugs for subclass with additional field. |
| hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedBlockBasedTableConfig.java | Introduces FORMAT_VERSION=5 constant for downgrade-safe SST compatibility. |
| hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBTable.java | Adds unit test covering ByteBuffer existence-check fallback behavior. |
| hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBStoreCodecBufferIterator.java | Updates Mockito matchers to be explicit with ByteBuffer types. |
| hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java | Clarifies that existence/fast-path APIs must remain definitive vs point lookups. |
| hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java | Duplicates ByteBuffer for keyMayExist safety and migrates SST deletion to range deletion with CF-handle lookup. |
| hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBTable.java | Updates isExist/getIfExist to treat value-less “may exist” as inconclusive and confirm via point-get; fixes ByteBuffer duplication. |
| hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBProfile.java | Applies pinned SST format version to block-based table configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TestRocksDBCheckpointDiffer: getSSTDiffList returns HashMap values, whose order is not guaranteed and differs between the all-tables baseline call and each per-subset call (they build independent maps from different input sets). Compare with containsExactlyInAnyOrderElementsOf instead of the order-sensitive containsExactlyElementsOf to avoid spurious failures. TestRDBTable: add coverage for the RocksDB "exists with value" fast path of getIfExist(ByteBuffer, ByteBuffer), asserting the returned length, that the value written through outValue.duplicate() is visible in the caller's buffer, and that the fast path does not fall back to a point-get. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ace override Since RocksDB 9.x, DBOptions#setLogger takes a LoggerInterface, so the existing setLogger(Logger) method was only an overload, not an override. A caller invoking setLogger through a DBOptions-typed reference would hit the parent method directly and bypass the leak tracking, leaking the previously set logger. Override setLogger(LoggerInterface) instead so every call path is tracked, and close the previous/current logger when it owns native resources. Add TestManagedDBOptions verifying the logger is closed on replace and on close, including when setLogger is called via a DBOptions reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The subset assertions compare getSSTDiffList's output against a baseline that is itself produced by getSSTDiffList, so a systematic bug returning the wrong files for every input would corrupt both sides and still pass. The only independent check was that at least one diff is non-empty. Add a structural invariant not derived from getSSTDiffList's own output: a snapshot diffed against itself must be empty. Together with the existing non-empty guard, this bounds a broken diff in both directions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment justified the last-level guard by the RocksDB#deleteFile API restriction, but the method now uses deleteSstFileRange, which has no such restriction. Update the comment to note the guard is retained to preserve existing pruning behavior rather than because of an API limitation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Note that leakTracker is not part of the filter's identity, so delegating to BloomFilter's equality is correct and the override exists only to declare it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I have rebased the PR, address all review comments, did self-review with Opus 4.8 and addressed issues found. CI has passed in my fork. Marking this ready for review. |
jojochuang
left a comment
There was a problem hiding this comment.
BTW, we have a few test files using format_version=2 e.g.
that shouldn't be a problem though.
| public RDBSstFileWriter(File externalFile) throws RocksDatabaseException { | ||
| // Pin the SST format version so files written here (e.g. snapshot defrag | ||
| // ingest) stay readable if Ozone is downgraded before finalization. | ||
| tableConfig.setFormatVersion(ManagedBlockBasedTableConfig.FORMAT_VERSION); |
There was a problem hiding this comment.
Need an upgrade test later. We'll need it for 2.3.0 release anyway.
Actually, we're missing upgrade tests for 2.1 and 2.2 too https://github.com/apache/ozone/tree/master/hadoop-ozone/dist/src/main/compose/upgrade/upgrades/non-rolling-upgrade/callbacks
There was a problem hiding this comment.
Let's file JIRAs for those.
Filed https://issues.apache.org/jira/browse/HDDS-15769 for 2.1 and 2.2
|
Merged. Thanks @adoroszlai @ptlrs @jojochuang for the reviews. |
Generated-by: GPT-5.3-Codex, GPT-5.4, Claude Opus 4.8
What changes were proposed in this pull request?
Bump RocksDB version from 7.7.3 to
10.4.210.10.1. The goal is to upgrade rocksdbjni while keepingon-disk format and API semantics unchanged.
DO NOT MERGE until v10.9.0 or higher RocksDB JNI is available.Note: Potential forward compatibility bug < v10.9.0: https://github.com/facebook/rocksdb/releases/tag/v10.9.13. Currently in this PR I am using a rocksdbjni 10.10.1 fatjar I built myself and pushed to maven central snapshot. We need to properly publish it (for example, under Apache Ozone account) before we can merge this.4.
RocksDB#deleteFileis removed upstream (facebook/rocksdb#13322). Callers are migrated todeleteFilesInRangesviaManagedRocksDB.deleteSstFileRange.5. Adapted RDBTable.isExist/getIfExist to the 9.10.0 KeyMayExist contract by treating a value-less "may exist" hit as inconclusive and confirming with a point-get, and by passing key.duplicate() on the ByteBuffer paths (since KeyMayExist can advance the buffer position), keeping these Ozone existence checks definitive with no behavior change.
hdds-rocks-native. Theraw_sst_file_readerpatch is updated for RocksDB 10.x, and the upstream source version is derived fromrocksdb.version(stripping the packaging suffix10.10.1.1->10.10.1) so the source download matches the correct upstream tag.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14225
How was this patch tested?