feat: remove build tag for pebbledb #117
Conversation
WalkthroughThe changes in this pull request encompass updates to various files, including the Changes
Poem
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 (
|
![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 (3)
Makefile (1)
6-7: Consider updating the comment to reflect the changeThe comment above the test target mentions "By default this will only test memdb, goleveldb, and pebbledb", but since pebbledb tests now run without tags, this comment should be updated.
-## By default this will only test memdb, goleveldb, and pebbledb, which do not require cgo +## By default this will run all tests except those requiring cgo (like rocksdb)db.go (1)
20-20: Fix documentation typoThere's a duplicate "DB" in the comment. Change "PebbleDBDBBackend" to "PebbleDBBackend" to maintain consistency with the actual constant name.
-// PebbleDBDBBackend represents pebble (uses github.com/cockroachdb/pebble) +// PebbleDBBackend represents pebble (uses github.com/cockroachdb/pebble)pebble.go (1)
Line range hint
144-148: Consider documenting performance implicationsThe sync/nosync behavior controlled by ForceSync affects write performance in Set, Delete, and Write operations. Consider adding a comment about the performance trade-offs between durability and speed when ForceSync is enabled.
Example documentation:
var ( + // ForceSync controls write durability. When set to "1", all write operations + // use Sync mode, providing stronger durability guarantees at the cost of + // slower write performance. Default "0" uses NoSync for better performance. ForceSync = "0" isForceSync = false )Also applies to: 182-186, 284-288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)Makefile(1 hunks)db.go(1 hunks)go.mod(1 hunks)pebble.go(2 hunks)pebble_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- pebble_test.go
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~6-~6: Adverb repetition.
Context: ...tch` interfaces more flexible by a type alias * Remove alias for PebbleDB ## [v1.0.2] - 2024-02-26 ...
(ADVERB_VERB_ADVERB_REPETITION)
🔇 Additional comments (8)
Makefile (1)
7-7: Verify the impact of removing pebbledb build tag
The simplified test command aligns with removing the pebbledb build tag. However, let's verify that all pebble-related tests are properly configured to run without the tag.
✅ Verification successful
Removing pebbledb build tag is consistent with codebase changes
The removal of the -tags pebbledb flag from the test command is appropriate because:
- The PR removes dedicated pebble-specific test targets (
test-pebbleandtest-all) - Pebble-related files (
pebble.go,pebble_test.go) remain in the codebase - No build tag constraints for pebble were found in the codebase
- The change aligns with the Makefile comment that states tests will run for "memdb, goleveldb, and pebbledb" by default
This indicates that pebble tests are now part of the default test suite without requiring explicit build tags, which is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining build tag dependencies and test configurations
# Check for any remaining pebbledb build tags
echo "Checking for any remaining pebbledb build tags..."
rg "//go:build.*pebbledb"
# Check for test files that might be affected
echo "Checking for test files that might need updates..."
rg -l "pebbledb" --type go --type test
Length of output: 315
Script:
#!/bin/bash
# Let's try a different approach to check for pebble-related tests and build tags
# Check for any remaining build tags in go files
echo "Checking for build tags..."
rg "//\s*\+build.*pebble"
# Check for pebble-related test files
echo "Checking for pebble-related test files..."
fd -e go -x rg -l "pebble" {}
# Check Makefile history to understand the context better
echo "Checking Makefile changes..."
git diff HEAD^1 Makefile
Length of output: 1008
db.go (1)
20-22: LGTM! Removal of build tags aligns with making pebble a standard backend
The simplification of the PebbleDB constant definition and its documentation aligns well with the PR objective of removing the build tag for pebbledb. This change effectively promotes pebble to be a standard backend alongside goleveldb and others.
go.mod (4)
14-15: Verify pinned goleveldb dependency.
The comment explains why syndtr/goleveldb is pinned to a specific version. This is good practice for documenting version constraints.
18-44: Review indirect dependency updates for security and compatibility.
Significant version jumps in indirect dependencies:
- DataDog/zstd: v1.4.5 -> v1.5.6
- getsentry/sentry-go: v0.18.0 -> v0.29.1
- klauspost/compress: v1.15.15 -> v1.17.11
- prometheus packages updated to latest versions
✅ Verification successful
Based on the changelog information and security vulnerability check results, I can now provide a final response:
Indirect dependency updates appear safe to proceed with
- No security vulnerabilities found in the updated dependencies
- Changes are primarily bug fixes and improvements:
- DataDog/zstd (v1.5.6): Fixes decompression buffer sizing and large payload handling
- getsentry/sentry-go (v0.29.1): Contains error correlation fixes and trace context improvements
- klauspost/compress (v1.17.11): Includes fixes for CRC handling and content-type headers
Note: The breaking change in sentry-go v0.29.0 (removal of sentrymartini integration) doesn't impact this project as the integration isn't used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known vulnerabilities in the updated dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/DataDog/zstd") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}'
# Check for any deprecated APIs or breaking changes
for pkg in "DataDog/zstd" "getsentry/sentry-go" "klauspost/compress"; do
echo "Checking $pkg changelog..."
gh api repos/${pkg}/releases --jq '.[0:2].[].body'
done
Length of output: 7250
3-5: Verify Go version and toolchain compatibility.
The toolchain version (go1.23.1) is newer than the Go version (1.22.0) specified. While this works, it's unconventional and might cause confusion. Consider either:
- Upgrading Go version to 1.23
- Or removing the toolchain directive if not specifically needed
✅ Verification successful
Go version and toolchain configuration appears safe to use
The codebase doesn't use any Go 1.22-specific features, and the build constraints are only used for conditional compilation of RocksDB-related code. While the toolchain version (1.23.1) is newer than the Go version (1.22.0), this is actually a supported configuration as the toolchain is backward compatible. The Go version specified is sufficient for the current codebase needs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any Go files use features requiring Go 1.22 or newer
rg -l 'any|clear|min|max|clear|contains|copy|deleteFunc|indexOf|lastIndexOf' --type go
# Check for potential compatibility issues in dependencies
go list -m -json all | jq -r '.Path + " " + .Version' | while read -r dep ver; do
if [[ $ver == *"v0.0.0"* ]]; then
echo "Pseudo-version detected: $dep $ver"
fi
done
Length of output: 419
Script:
#!/bin/bash
# Let's check for specific Go 1.22+ features more precisely
rg -A 2 '\bslices\.' --type go # slices package was expanded in 1.22
rg -A 2 '\bmaps\.' --type go # maps package was expanded in 1.22
rg -A 2 '\bclear\(' --type go # clear builtin was added in 1.22
rg -A 2 'range.*clear' --type go # range clear syntax was added in 1.22
# Check if there are any build constraints or minimum Go version requirements
rg -l '^//\s*\+build' --type go
rg -l '^//go:build' --type go
Length of output: 324
Script:
#!/bin/bash
# Let's check the content of build constraints to understand version requirements
rg '^//go:build' -A 1 rocksdb*.go
# Check for any direct usage of Go 1.22+ features in the codebase
# Looking for specific method calls that were introduced in 1.22
rg '\b(slices\.(Max|Min|Clone|Contains)|maps\.(Clone|Copy|DeleteFunc)|clear\()' --type go
Length of output: 464
8-12: Verify breaking changes in core dependency updates.
Several direct dependencies have been updated with potentially breaking changes:
- pebble: v1.1.0 -> v1.1.2
- grocksdb: v1.8.12 -> v1.9.7
- btree: v1.1.2 -> v1.1.3
✅ Verification successful
Let me gather more information about how these dependencies are used in the codebase to better assess the impact of the updates.
Let me check the actual API usage to ensure there are no breaking changes.
Minor version updates appear safe to proceed
Based on the analysis of dependency usage and version changes:
- Pebble v1.1.0 -> v1.1.2: Minor version update with dependency updates and MIPS fix
- grocksdb v1.8.12 -> v1.9.7: Adapts to RocksDB 9.7.3, adds missing read tiers, no breaking API changes visible
- btree v1.1.2 -> v1.1.3: Minor version update with no changelog indicating breaking changes
The codebase uses standard APIs from these libraries (DB operations, iterators, options) that are stable across these minor version updates. No deprecated or removed API usage patterns were detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any API changes in the updated dependencies
for pkg in "github.com/cockroachdb/pebble" "github.com/linxGnu/grocksdb" "github.com/google/btree"; do
echo "Checking $pkg changelog/releases..."
gh api repos/${pkg#github.com/}/releases --jq '.[0:2].[].body'
done
Length of output: 1616
Script:
#!/bin/bash
# Check usage of pebble, grocksdb, and btree in the codebase
echo "Checking pebble usage..."
rg "github.com/cockroachdb/pebble" -B 2 -A 2
echo -e "\nChecking grocksdb usage..."
rg "github.com/linxGnu/grocksdb" -B 2 -A 2
echo -e "\nChecking btree usage..."
rg "github.com/google/btree" -B 2 -A 2
# Check for any test failures or compilation issues in recent commits
echo -e "\nChecking recent commit messages for any related issues..."
git log --grep="pebble\|grocksdb\|btree" --since="1 month ago" --oneline
Length of output: 5343
Script:
#!/bin/bash
# Check specific API usage patterns for each dependency
echo "Checking pebble API usage..."
ast-grep --pattern 'pebble.$_'
echo -e "\nChecking grocksdb API usage..."
ast-grep --pattern 'grocksdb.$_'
echo -e "\nChecking btree API usage..."
ast-grep --pattern 'btree.$_'
# Also check imports to see the scope of usage
echo -e "\nChecking import patterns..."
rg "^import.*pebble|^import.*grocksdb|^import.*btree" -A 3
Length of output: 2185
pebble.go (2)
35-35: LGTM: Clear upgrade instructions
The installation commands correctly demonstrate the upgrade process, showing when to use ForceSync during upgrades and when it can be omitted for normal operation.
Also applies to: 45-45
Line range hint 53-54: LGTM: Clean ForceSync implementation
The ForceSync configuration is well-implemented with:
- Thread-safe initialization at startup
- Efficient caching of the parsed value
- Clear boolean flag for runtime checks
Also applies to: 61-65
julienrbrt
changed the title
julienrbrt
changed the title
| github.com/cockroachdb/pebble v1.1.0 | ||
| github.com/google/btree v1.1.2 | ||
| github.com/cockroachdb/pebble v1.1.2 | ||
| github.com/google/btree v1.1.3 | ||
| github.com/linxGnu/grocksdb v1.8.12 |
There was a problem hiding this comment.
cometbft-db bumped this, we may not want to do that in the same release line as it may break stuff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
5-6: Consider improving grammar in the interface change description.The article usage could be improved for better clarity.
-* Make `Iteractor` and `Batch` interfaces more flexible by a type alias +* Make `Iteractor` and `Batch` interfaces more flexible by the type alias
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... more flexible by a type alias * Remove build tag for PebbleDB ## [v1.0.2] - 2024-02...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (1)
CHANGELOG.md (1)
3-3: LGTM!
The version header follows proper semantic versioning format and ISO 8601 date standard.
Helps cosmos/cosmos-sdk#22577.
CometBFT / Comet-DB now has pebbledb by default this now aligns with this so that our users do not need a build flag.
Summary by CodeRabbit
Release Notes for Version 1.1.0
New Features
IteractorandBatchinterfaces.PebbleDBBackendas a valid database backend type.Bug Fixes
Documentation
Chores