Problem: rocksdb 9.8.4 is not used #1712
Conversation
WalkthroughThe changes in this pull request include an update to the changelog, introducing a new section titled "UNRELEASED" that details improvements, particularly the upgrade of the RocksDB dependency to version Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
=======================================
Coverage 16.87% 16.88%
=======================================
Files 72 72
Lines 6163 6167 +4
=======================================
+ Hits 1040 1041 +1
- Misses 5000 5002 +2
- Partials 123 124 +1 |
mmsqe
requested review from
JayT106 and
devashishdxt
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
nix/rocksdb.nix (1)
Line range hint
1-150: Consider documenting version-specific build requirements.Given the complexity of RocksDB's build configuration and platform-specific requirements, consider:
- Adding comments explaining version-specific patches and their removal criteria
- Creating a version compatibility matrix in the repository's documentation
- Documenting any performance improvements or breaking changes from this upgrade
This will help future maintainers understand the rationale behind build configurations and make future upgrades smoother.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)nix/rocksdb.nix(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🔇 Additional comments (4)
nix/rocksdb.nix (4)
24-24: Verify RocksDB version hash and stability.
The version upgrade to 9.8.4 looks good, but let's verify the source hash matches the official release.
Also applies to: 30-30
✅ Verification successful
RocksDB version 9.8.4 is verified and stable
The version upgrade from 9.7.4 to 9.8.4 has been verified:
- Release tag exists with commit hash
48b90442a278d47e9092274a80d00368e3def7fe - Release notes show it's a recent stable release (11/18/2024)
- Changes are primarily bug fixes and behavior improvements:
- Remote compaction OPTIONS file handling
- No breaking changes or critical issues reported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RocksDB version hash and check release notes
# Get the hash of the tagged release
gh api repos/facebook/rocksdb/git/refs/tags/v9.8.4 | jq -r '.object.sha'
# Check release notes for any breaking changes or known issues
curl -s https://raw.githubusercontent.com/facebook/rocksdb/v9.8.4/HISTORY.md | head -n 50
Length of output: 5123
Line range hint 53-71: Verify CMake flags compatibility with RocksDB 9.8.4.
The CMake configuration looks comprehensive, but we should verify if the new version introduces or deprecates any build flags.
✅ Verification successful
This script will help us see the available CMake options in version 9.8.4 directly, which we can compare with our current configuration.
CMake configuration is compatible with RocksDB 9.8.4
After reviewing the available CMake options in RocksDB 9.8.4, all the flags used in the current configuration (rocksdb.nix) are valid and properly set:
- Core flags like
PORTABLE,WITH_JEMALLOC,WITH_JNIare present - Compression-related flags (
WITH_BZ2,WITH_LZ4,WITH_SNAPPY,WITH_ZLIB,WITH_ZSTD) are supported - Tool-related flags (
WITH_BENCHMARK_TOOLS,WITH_TOOLS,WITH_CORE_TOOLS) match the available options USE_RTTIis still a valid configuration option
The current CMake configuration remains compatible with the new version, and no new mandatory flags need to be added.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any CMake changes between versions
# Compare CMake files between versions
curl -s https://raw.githubusercontent.com/facebook/rocksdb/v9.7.4/CMakeLists.txt > cmake_9.7.4.txt
curl -s https://raw.githubusercontent.com/facebook/rocksdb/v9.8.4/CMakeLists.txt > cmake_9.8.4.txt
diff -u cmake_9.7.4.txt cmake_9.8.4.txt | grep -E '^\+.*CMAKE|^\-.*CMAKE'
Length of output: 241
Script:
#!/bin/bash
# Let's try a different approach to check CMake options directly from the source
curl -s https://raw.githubusercontent.com/facebook/rocksdb/v9.8.4/CMakeLists.txt | grep -E "^option\(|^set\(.*CACHE"
Length of output: 2065
Line range hint 82-106: Review patch requirements for version 9.8.4.
The file contains version-specific patches for GCC 13 compatibility. Since we're moving to 9.8.4, some of these patches might no longer be necessary as they were fixed upstream.
Line range hint 108-122: Verify platform-specific requirements for version 9.8.4.
The file includes platform-specific handling for Darwin, Linux, and Windows. Let's ensure these configurations are still valid for 9.8.4.
✅ Verification successful
Let me analyze the platform-specific changes from the commit history and generate additional verification scripts to check for any platform-specific changes that might affect the configuration.
The first command will show platform-specific conditionals in CMakeLists.txt, the second will show platform detection logic, and the third will list platform-specific files. This will help verify if there are any significant platform-specific changes that would affect the configuration in rocksdb.nix.
Let me try a different approach to verify the platform-specific configurations:
These commands will help us verify the platform-specific handling in the Nix build configuration files, which is more relevant to the review comment's context.
Based on the search results, I can now provide a final response about the platform-specific configurations:
Platform-specific configurations are correctly maintained for version 9.8.4
The platform-specific handling in rocksdb.nix remains valid and necessary:
- Darwin (macOS) configuration correctly handles library path fixing using
install_name_tool - Linux shared library handling properly sets rpath using
patchelf - Windows-specific handling is properly guarded for MinGW builds
The configurations align with RocksDB's platform requirements and no changes are needed for version 9.8.4.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for platform-specific changes
# Look for platform-specific changes in the release
curl -s https://api.github.com/repos/facebook/rocksdb/commits?path=CMakeLists.txt&since=2023-01-01 | \
jq -r '.[] | select(.commit.message | contains("darwin") or contains("windows") or contains("linux")) | .commit.message'
Length of output: 69860
Script:
#!/bin/bash
# Check for platform-specific changes in CMakeLists.txt
rg -A 5 -B 5 "if.*darwin|if.*linux|if.*windows" CMakeLists.txt
# Check for platform-specific changes in build tools
fd -t f "build_detect_platform" -x cat {}
# Look for platform-specific files
fd -t f ".*darwin.*|.*linux.*|.*windows.*"
Length of output: 208
Script:
#!/bin/bash
# Search for platform-specific code in the entire codebase
rg -A 3 "stdenv\.(isDarwin|isLinux|isWindows)" nix/
# Check for platform-specific changes in the rocksdb.nix file
rg -A 3 "(darwin|linux|windows)" nix/rocksdb.nix
# Look for platform-specific build configurations
rg -A 3 "platform.*extensions" nix/
Length of output: 2303
* Problem: rocksdb 9.8.4 is not used (#1712) for more info, facebook/rocksdb@v9.7.4...v9.8.4 * Problem: query blocks before enable feemarket module get nil pointer error (#1714) * Problem: unable to query historical txs whose module is removed (#1713) * Problem: unable to query historical txs from deleted icaauth module * test * poetry * deps * add unit tets * cleanup * rename * revert integration test * revert * add authz * changelog * test authz * cleanup * register in query * only patch encoding for tx service * Revert "only patch encoding for tx service" This reverts commit 4b1c141. --------- Co-authored-by: HuangYi <[email protected]> --------- Co-authored-by: HuangYi <[email protected]>
for more info, facebook/rocksdb@v9.7.4...v9.8.4
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
9.8.4, enhancing performance and stability.