cache first version for legacy versions #1018
Conversation
WalkthroughThe pull request introduces several updates primarily focused on the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (1)
nodedb.go (1)
741-741: Consider adding a comment explaining the caching behavior.Add a comment explaining that this caches the first version for performance optimization when dealing with legacy versions.
+ // Cache the first version from legacy format to avoid repeated database queries ndb.resetFirstVersion(version)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)nodedb.go(1 hunks)
🔇 Additional comments (2)
CHANGELOG.md (1)
17-17: LGTM: Clear and accurate changelog entry.
The changelog entry accurately describes the change and its impact.
nodedb.go (1)
741-741: LGTM: Good performance optimization, but verify thread safety.
The addition of caching the first version when found in legacy format is a good optimization that fixes the performance regression. The change is consistent with the changelog entry.
However, let's verify the thread safety of this caching mechanism:
✅ Verification successful
Let me gather more information about mutex usage patterns around firstVersion to ensure thread safety.
Let me check one final aspect about the getFirstVersion implementation to complete the thread safety analysis.
Thread safety verified for firstVersion caching
The implementation is thread-safe:
resetFirstVersionuses proper mutex locking withLock()anddefer Unlock()getFirstVersioncorrectly locks during firstVersion read- All modifications to
firstVersionare protected by mutex
The only minor concern is in orphans() method which reads firstVersion without a lock, but this is acceptable since:
- It's used only for iteration bounds
- Concurrent updates would not cause corruption, only potentially skip/include an extra version
- The method appears to be used for maintenance/debugging purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in version management
# Look for other concurrent access to firstVersion
rg -A 5 "firstVersion\s*=|firstVersion\s*==|firstVersion\s*!=|firstVersion\s*[<>]"
# Look for other calls to resetFirstVersion
rg "resetFirstVersion"
# Look for mutex usage patterns around firstVersion access
ast-grep --pattern 'ndb.mtx.Lock()
$$$
ndb.firstVersion
$$$'
Length of output: 2411
Script:
#!/bin/bash
# Check mutex patterns around firstVersion access and modification
# Look for the firstVersion field declaration and surrounding context
rg -A 5 -B 5 "firstVersion.*int64"
# Look for all getFirstVersion method implementations
ast-grep --pattern 'func $_getFirstVersion() (int64, error) {
$$$
}'
# Look for mutex patterns in resetFirstVersion
ast-grep --pattern 'func (ndb *nodeDB) resetFirstVersion($_) {
$$$
}'
Length of output: 2489
Script:
#!/bin/bash
# Check getFirstVersion implementation for mutex patterns
# Look for getFirstVersion implementation
ast-grep --pattern 'func (ndb *nodeDB) getFirstVersion() (int64, error) {
$$$
}'
# Look for all firstVersion reads
rg -A 3 -B 3 "ndb\.firstVersion"
Length of output: 2455
cache first version for legacy versions (backport: cosmos#1018)
|
@mergify backport release/v1.2.x |
✅ Backports have been created
|
(cherry picked from commit e60601d) # Conflicts: # CHANGELOG.md
|
@Mergifyio backport release/v1.3.x |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
✅ Backports have been created
|
(cherry picked from commit e60601d) # Conflicts: # CHANGELOG.md
fix performance regression after upgrade.
Context
We see higher cpu usage in rpc nodes when serving similar traffic of queries, we dump a golang trace and see following stacktrace appears frequently, we suspect this to be the issue.
The first version in ndb seems should be cached, but it's not in legacy version case, which cause an extra seek call to underlying db, and the extra call happens for each store and for each query request.
We've verified this fix reduce the cpu usage on the nodes quite effective.
Summary by CodeRabbit
New Features
DeleteVersionsFrom(int64)andGetLatestVersion.Bug Fixes
GetNode.Improvements
Documentation