fix: get latest version in store after legacy pruning fails#1067
Conversation
WalkthroughThis change introduces a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant nodeDB
Client->>nodeDB: deleteVersionsTo()
nodeDB->>nodeDB: getFirstNonLegacyVersion()
nodeDB->>nodeDB: getLatestVersion()
nodeDB-->>Client: Return deletion result or error
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (1)
nodedb_test.go (1)
449-488: Consider adopting sub-tests for clearer structure.By splitting each test scenario into a separate
t.Runblock, you'll have well-defined test boundaries and clearer logs if one case fails.Here is a quick example of using sub-tests:
-func TestGetFirstNonLegacyVersion(t *testing.T) { - // current test code ... +func TestGetFirstNonLegacyVersion(t *testing.T) { + t.Run("Empty database", func(t *testing.T) { + firstVersion, err := ndb.getFirstNonLegacyVersion() + require.NoError(t, err) + require.Equal(t, int64(0), firstVersion) + }) + + t.Run("Only legacy versions", func(t *testing.T) { + // create legacy version, call getFirstNonLegacyVersion, etc. + }) + + t.Run("Mixed legacy and non-legacy versions", func(t *testing.T) { + // create version 2 as non-legacy, verify result is 2... + }) + + // additional scenarios... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nodedb.go(2 hunks)nodedb_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Benchmarks
- GitHub Check: Test
🔇 Additional comments (2)
nodedb_test.go (1)
449-488: Comprehensive test coverage for multiple scenarios.This test method covers empty databases, only-legacy versions, mixed legacy and non-legacy, as well as multiple non-legacy versions. Each scenario is validated appropriately, ensuring
getFirstNonLegacyVersionbehaves consistently under different states. Good job!nodedb.go (1)
777-804: Binary search logic is efficient and robust.This implementation properly locks and unlocks before calling
getLatestVersion, then uses a standard binary search to find the earliest existing version. The approach appears correct and improves performance over a linear lookup.
|
@mergify backport release/v1.2.x |
|
@Mergifyio backport release/v1.2.x |
✅ Backports have been createdDetails
|
|
@Mergifyio backport release/v1.3.x |
✅ Backports have been createdDetails
|
(cherry picked from commit 00941d2)
(cherry picked from commit 00941d2)
2 participants
Description
Currently if legacy pruning fails it sets the
firstVersiontolatestLegacyVersion + 1That works under the assumption that if legacy pruning fails that the non-legacy node is the first version is available in the store, this is not the case, and the code will then linearly iterate until it finds the next version, which could be a long wait.
Context
please read #1063 for context on this issue
What this function does
What should actually happen
Summary by CodeRabbit
New Features
Tests