Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow saving version 0 #1002

Merged
merged 9 commits into from
Nov 4, 2024
Merged

feat: allow saving version 0 #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b7bfb3a
feat: allow saving version 0
kocubinski Oct 31, 2024
35f6e94
add options
kocubinski Oct 31, 2024
81b4a52
more loc but safer
kocubinski Oct 31, 2024
507b04b
revert some
kocubinski Oct 31, 2024
f37eaef
rm comment
kocubinski Oct 31, 2024
f1d176e
revert committed binary
kocubinski Oct 31, 2024
cfce4c6
fix test, rm legacydump binary
kocubinski Oct 31, 2024
ec511e4
fix usage in docs
kocubinski Oct 31, 2024
fbd5df5
fix for load version
kocubinski Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ jobs:
# integer overflow).
- name: test & coverage report creation
run: |
cd cmd/legacydump && go build -o legacydump main.go && cd ../..
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve the binary build step

The current approach of changing directories to build the binary has several issues:

  1. Could leave the working directory in wrong state if build fails
  2. Creates the binary in the source directory
  3. Uses multiple commands where one would suffice
- cd cmd/legacydump && go build -o legacydump main.go && cd ../..
+ go build -o /tmp/legacydump ./cmd/legacydump

Additionally, consider adding a cleanup step in a post block to ensure the binary is removed after tests:

    post:
      - name: Cleanup
        if: always()
        run: rm -f /tmp/legacydump

go test ./... -mod=readonly -timeout 10m -short -race -coverprofile=coverage.txt -covermode=atomic
go test ./... -mod=readonly -timeout 15m
GOARCH=386 go test ./... -mod=readonly -timeout 15m
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
cmd/legacydump/legacydump
vendor
.glide
*.swp
Expand Down
7 changes: 5 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ test-short:
@go test ./... $(LDFLAGS) -v --race --short
.PHONY: test-short

test:
cmd/legacydump/legacydump:
cd cmd/legacydump && go build -o legacydump main.go

test: cmd/legacydump/legacydump
@echo "--> Running go test"
@go test ./... $(LDFLAGS) -v
@go test ./... $(LDFLAGS)
Comment on lines +30 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Removing --short flag will significantly impact test execution time

The codebase contains several time-intensive tests that are specifically skipped in short mode:

  • Concurrent operation tests in mutable_tree_test.go that are explicitly skipped in short mode
  • Large-scale tests with significant iterations:
    • Tests with 1000 random key/value entries per version
    • Performance test with 1,000,000 iterations
    • Batch tests with 10MB of data (1000 entries of 10KB each)
    • Basic tests with 10,000 iterations

Removing the --short flag will cause these tests to run, substantially increasing the CI pipeline duration. Consider keeping a separate make target for comprehensive testing (current test target) while maintaining the test-short target for faster CI runs.

🔗 Analysis chain

Verify test execution time impact.

The removal of the --short flag from go test means all tests will run, including long-running ones. While this provides better coverage, it might significantly increase the CI/CD pipeline duration.

Let's check the test execution time impact:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare test execution time with and without --short flag

echo "Running tests with --short flag..."
time go test ./... --short >/dev/null

echo -e "\nRunning tests without --short flag..."
time go test ./... >/dev/null

Length of output: 433

Script:

#!/bin/bash
# Let's check for test files and analyze their content for long-running tests
# Look for test files with potential time-consuming operations

# Search for test files containing sleep, timeout, or other time-related patterns
echo "=== Test files with potential long-running operations ==="
rg -l --type go "time\.(Sleep|After|Tick)|context\.WithTimeout" ".*_test\.go$"

echo -e "\n=== Test files with skip conditions based on short flag ==="
rg -l --type go "testing\.Short\(\)" ".*_test\.go$"

echo -e "\n=== Detailed context of tests using Short() flag ==="
rg -B2 -A2 --type go "testing\.Short\(\)" ".*_test\.go$"

Length of output: 733

Script:

#!/bin/bash
# Let's try a different approach to find test files
echo "=== Finding Go test files ==="
fd "_test\.go$"

echo -e "\n=== Checking Makefile history for test-related changes ==="
git log -p -- Makefile | grep -A 5 -B 5 "go test"

Length of output: 3363

Script:

#!/bin/bash
# Let's analyze the test files for time-consuming operations and short flag usage
echo "=== Checking test files for time-consuming operations ==="
rg -C3 "time\.(Sleep|After|Tick)|context\.WithTimeout" basic_test.go batch_test.go tree_test.go mutable_tree_test.go

echo -e "\n=== Checking test files for testing.Short() usage ==="
rg -C3 "testing\.Short\(\)" basic_test.go batch_test.go tree_test.go mutable_tree_test.go

echo -e "\n=== Checking for large test data or iterations ==="
rg -C3 "for.+range|for.+:=.+;.+;" basic_test.go batch_test.go tree_test.go mutable_tree_test.go | rg "1000|10000"

Length of output: 2610

.PHONY: test

format:
Expand Down
Binary file removed cmd/legacydump/legacydump
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion docs/node/nodedb.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ When a version `v` is deleted, all nodes which removed in the current version wi
```golang
// DeleteVersionsFrom permanently deletes all tree versions from the given version upwards.
func (ndb *nodeDB) DeleteVersionsFrom(fromVersion int64) error {
latest, err := ndb.getLatestVersion()
_, latest, err := ndb.getLatestVersion()
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion immutable_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ func (t *ImmutableTree) IsFastCacheEnabled() (bool, error) {
}

func (t *ImmutableTree) isLatestTreeVersion() (bool, error) {
latestVersion, err := t.ndb.getLatestVersion()
_, latestVersion, err := t.ndb.getLatestVersion()
kocubinski marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false, err
}
Expand Down
4 changes: 2 additions & 2 deletions iterator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func TestIterator_WithDelete_Full_Ascending_Success(t *testing.T) {
err = tree.DeleteVersionsTo(1)
require.NoError(t, err)

latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
immutableTree, err := tree.GetImmutable(latestVersion)
require.NoError(t, err)
Expand Down Expand Up @@ -253,7 +253,7 @@ func setupIteratorAndMirror(t *testing.T, config *iteratorTestConfig) (corestore
_, _, err := tree.SaveVersion()
require.NoError(t, err)

latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
immutableTree, err := tree.GetImmutable(latestVersion)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion migrate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func TestDeleteVersions(t *testing.T) {
// Test LoadVersionForOverwriting for the legacy version
err = tree.LoadVersionForOverwriting(int64(targetVersion))
require.NoError(t, err)
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
require.Equal(t, int64(targetVersion), latestVersion)
legacyLatestVersion, err := tree.ndb.getLegacyLatestVersion()
Expand Down
27 changes: 15 additions & 12 deletions mutable_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type MutableTree struct {
unsavedFastNodeRemovals *sync.Map // map[string]interface{} FastNodes that have not yet been removed from disk
ndb *nodeDB
skipFastStorageUpgrade bool // If true, the tree will work like no fast storage and always not upgrade fast storage
initialVersionSet bool

mtx sync.Mutex
}
Expand All @@ -62,6 +63,7 @@ func NewMutableTree(db corestore.KVStoreWithBatch, cacheSize int, skipFastStorag
unsavedFastNodeRemovals: &sync.Map{},
ndb: ndb,
skipFastStorageUpgrade: skipFastStorageUpgrade,
initialVersionSet: opts.initialVersionSet,
}
}

Expand All @@ -73,7 +75,8 @@ func (tree *MutableTree) IsEmpty() bool {

// GetLatestVersion returns the latest version of the tree.
func (tree *MutableTree) GetLatestVersion() (int64, error) {
return tree.ndb.getLatestVersion()
_, v, err := tree.ndb.getLatestVersion()
return v, err
}

// VersionExists returns whether or not a version exists.
Expand All @@ -90,10 +93,13 @@ func (tree *MutableTree) VersionExists(version int64) bool {
if err != nil {
return false
}
latestVersion, err := tree.ndb.getLatestVersion()
found, latestVersion, err := tree.ndb.getLatestVersion()
if err != nil {
return false
}
if !found {
return false
}

return firstVersion <= version && version <= latestVersion
}
Expand All @@ -104,7 +110,7 @@ func (tree *MutableTree) AvailableVersions() []int {
if err != nil {
return nil
}
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
if err != nil {
return nil
}
Expand Down Expand Up @@ -146,7 +152,7 @@ func (tree *MutableTree) WorkingHash() []byte {

func (tree *MutableTree) WorkingVersion() int64 {
version := tree.version + 1
if version == 1 && tree.ndb.opts.InitialVersion > 0 {
if version == 1 && tree.initialVersionSet {
version = int64(tree.ndb.opts.InitialVersion)
}
return version
Expand Down Expand Up @@ -449,21 +455,16 @@ func (tree *MutableTree) LoadVersion(targetVersion int64) (int64, error) {
tree.ndb.opts.InitialVersion, firstVersion)
}

latestVersion, err := tree.ndb.getLatestVersion()
ok, latestVersion, err := tree.ndb.getLatestVersion()
if err != nil {
return 0, err
}

if firstVersion > 0 && firstVersion < int64(tree.ndb.opts.InitialVersion) {
return latestVersion, fmt.Errorf("initial version set to %v, but found earlier version %v",
tree.ndb.opts.InitialVersion, firstVersion)
}

if latestVersion < targetVersion {
return latestVersion, fmt.Errorf("wanted to load target %d but only found up to %d", targetVersion, latestVersion)
}

if firstVersion == 0 {
if !ok {
if targetVersion <= 0 {
if !tree.skipFastStorageUpgrade {
tree.mtx.Lock()
Expand Down Expand Up @@ -604,7 +605,7 @@ func (tree *MutableTree) enableFastStorageAndCommit() error {
return err
}

latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
if err != nil {
return err
}
Expand Down Expand Up @@ -707,6 +708,7 @@ func (tree *MutableTree) UnsetCommitting() {
// the tree. Returns the hash and new version number.
func (tree *MutableTree) SaveVersion() ([]byte, int64, error) {
version := tree.WorkingVersion()
tree.initialVersionSet = false

if tree.VersionExists(version) {
// If the version already exists, return an error as we're attempting to overwrite.
Expand Down Expand Up @@ -871,6 +873,7 @@ func (tree *MutableTree) saveFastNodeRemovals() error {
// and is otherwise ignored.
func (tree *MutableTree) SetInitialVersion(version uint64) {
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider adding mutex protection for thread safety

The SetInitialVersion method modifies shared state (initialVersionSet) without synchronization. Consider protecting this operation with the existing mtx mutex to prevent potential race conditions in concurrent usage scenarios.

Apply this diff to add mutex protection:

 func (tree *MutableTree) SetInitialVersion(version uint64) {
+    tree.mtx.Lock()
+    defer tree.mtx.Unlock()
     tree.ndb.opts.InitialVersion = version
     tree.initialVersionSet = true
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
tree.initialVersionSet = true
func (tree *MutableTree) SetInitialVersion(version uint64) {
tree.mtx.Lock()
defer tree.mtx.Unlock()
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
}

}

// DeleteVersionsTo removes versions upto the given version from the MutableTree.
Expand Down
19 changes: 17 additions & 2 deletions mutable_tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,8 @@ func TestUpgradeStorageToFast_LatestVersion_Success(t *testing.T) {
require.False(t, isUpgradeable)
require.NoError(t, err)

_, _, err = tree.SaveVersion()
require.NoError(t, err)
isFastCacheEnabled, err = tree.IsFastCacheEnabled()
require.NoError(t, err)
require.True(t, isFastCacheEnabled)
Expand Down Expand Up @@ -907,7 +909,7 @@ func TestFastStorageReUpgradeProtection_NoForceUpgrade_Success(t *testing.T) {

// Pretend that we called Load and have the latest state in the tree
tree.version = latestTreeVersion
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
require.Equal(t, latestVersion, int64(latestTreeVersion))

Expand Down Expand Up @@ -1001,7 +1003,7 @@ func TestFastStorageReUpgradeProtection_ForceUpgradeFirstTime_NoForceSecondTime_

// Pretend that we called Load and have the latest state in the tree
tree.version = latestTreeVersion
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
require.Equal(t, latestVersion, int64(latestTreeVersion))

Expand Down Expand Up @@ -1479,3 +1481,16 @@ func TestMutableTreeClose(t *testing.T) {

require.NoError(t, tree.Close())
}

func TestMutableTree_InitialVersionZero(t *testing.T) {
db := dbm.NewMemDB()

tree := NewMutableTree(db, 0, false, NewNopLogger(), InitialVersionOption(0))

_, err := tree.Set([]byte("hello"), []byte("world"))
require.NoError(t, err)

_, version, err := tree.SaveVersion()
require.NoError(t, err)
require.Equal(t, int64(0), version)
}
27 changes: 14 additions & 13 deletions nodedb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ func (ndb *nodeDB) shouldForceFastStorageUpgrade() (bool, error) {
versions := strings.Split(ndb.storageVersion, fastStorageVersionDelimiter)

if len(versions) == 2 {
latestVersion, err := ndb.getLatestVersion()
_, latestVersion, err := ndb.getLatestVersion()
if err != nil {
// TODO: should be true or false as default? (removed panic here)
return false, err
kocubinski marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -525,7 +525,7 @@ func (ndb *nodeDB) deleteLegacyVersions(legacyLatestVersion int64) error {

// DeleteVersionsFrom permanently deletes all tree versions from the given version upwards.
func (ndb *nodeDB) DeleteVersionsFrom(fromVersion int64) error {
latest, err := ndb.getLatestVersion()
_, latest, err := ndb.getLatestVersion()
if err != nil {
return err
}
kocubinski marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -642,7 +642,7 @@ func (ndb *nodeDB) deleteVersionsTo(toVersion int64) error {
return err
}

latest, err := ndb.getLatestVersion()
_, latest, err := ndb.getLatestVersion()
if err != nil {
return err
}
kocubinski marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -727,7 +727,7 @@ func (ndb *nodeDB) getFirstVersion() (int64, error) {
return version, nil
}
// Find the first version
latestVersion, err := ndb.getLatestVersion()
_, latestVersion, err := ndb.getLatestVersion()
if err != nil {
return 0, err
}
Expand Down Expand Up @@ -797,21 +797,21 @@ func (ndb *nodeDB) resetLegacyLatestVersion(version int64) {
ndb.legacyLatestVersion = version
}

func (ndb *nodeDB) getLatestVersion() (int64, error) {
func (ndb *nodeDB) getLatestVersion() (bool, int64, error) {
ndb.mtx.Lock()
latestVersion := ndb.latestVersion
ndb.mtx.Unlock()

if latestVersion > 0 {
return latestVersion, nil
return true, latestVersion, nil
}

itr, err := ndb.db.ReverseIterator(
nodeKeyPrefixFormat.KeyInt64(int64(1)),
nodeKeyPrefixFormat.KeyInt64(int64(math.MaxInt64)),
)
if err != nil {
return 0, err
return false, 0, err
}
defer itr.Close()

Expand All @@ -821,24 +821,25 @@ func (ndb *nodeDB) getLatestVersion() (int64, error) {
nodeKeyFormat.Scan(k, &nk)
latestVersion = GetNodeKey(nk).version
ndb.resetLatestVersion(latestVersion)
return latestVersion, nil
return true, latestVersion, nil
}

if err := itr.Error(); err != nil {
return 0, err
return false, 0, err
}

// If there are no versions, try to get the latest version from the legacy format.
latestVersion, err = ndb.getLegacyLatestVersion()
if err != nil {
return 0, err
return false, 0, err
}
if latestVersion > 0 {
ndb.resetLatestVersion(latestVersion)
return latestVersion, nil
return true, latestVersion, nil
}

return 0, nil
return false, 0, nil
// return -1, nil
}

func (ndb *nodeDB) resetLatestVersion(version int64) {
Expand Down Expand Up @@ -1246,7 +1247,7 @@ func (ndb *nodeDB) traverseStateChanges(startVersion, endVersion int64, fn func(
if startVersion < firstVersion {
startVersion = firstVersion
}
latestVersion, err := ndb.getLatestVersion()
_, latestVersion, err := ndb.getLatestVersion()
if err != nil {
return err
kocubinski marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
4 changes: 2 additions & 2 deletions nodedb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func TestSetStorageVersion_Success(t *testing.T) {
ndb := newNodeDB(db, 0, DefaultOptions(), NewNopLogger())
require.Equal(t, defaultStorageVersionValue, ndb.getStorageVersion())

latestVersion, err := ndb.getLatestVersion()
_, latestVersion, err := ndb.getLatestVersion()
require.NoError(t, err)

err = ndb.SetFastStorageVersionToBatch(latestVersion)
Expand Down Expand Up @@ -404,7 +404,7 @@ func TestDeleteVersionsFromNoDeadlock(t *testing.T) {
err := ndb.SetFastStorageVersionToBatch(ndb.latestVersion)
require.NoError(t, err)

latestVersion, err := ndb.getLatestVersion()
_, latestVersion, err := ndb.getLatestVersion()
require.NoError(t, err)
require.Equal(t, expectedVersion+fastStorageVersionDelimiter+strconv.Itoa(int(latestVersion)), ndb.getStorageVersion())
require.NoError(t, ndb.batch.Write())
Expand Down
3 changes: 3 additions & 0 deletions options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ type Options struct {

// AsyncPruning is a flag to enable async pruning
AsyncPruning bool

initialVersionSet bool
}

// DefaultOptions returns the default options for IAVL.
Expand All @@ -105,6 +107,7 @@ func SyncOption(sync bool) Option {
func InitialVersionOption(iv uint64) Option {
return func(opts *Options) {
opts.InitialVersion = iv
opts.initialVersionSet = true
}
}

Expand Down
6 changes: 3 additions & 3 deletions tree_random_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func assertEmptyDatabase(t *testing.T, tree *MutableTree) {

storageVersionValue, err := tree.ndb.db.Get([]byte(firstKey))
require.NoError(t, err)
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
require.Equal(t, fastStorageVersionValue+fastStorageVersionDelimiter+strconv.Itoa(int(latestVersion)), string(storageVersionValue))

Expand Down Expand Up @@ -347,7 +347,7 @@ func assertMirror(t *testing.T, tree *MutableTree, mirror map[string]string, ver

// Checks that fast node cache matches live state.
func assertFastNodeCacheIsLive(t *testing.T, tree *MutableTree, mirror map[string]string, version int64) {
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
if latestVersion != version {
// The fast node cache check should only be done to the latest version
Expand All @@ -364,7 +364,7 @@ func assertFastNodeCacheIsLive(t *testing.T, tree *MutableTree, mirror map[strin

// Checks that fast nodes on disk match live state.
func assertFastNodeDiskIsLive(t *testing.T, tree *MutableTree, mirror map[string]string, version int64) {
latestVersion, err := tree.ndb.getLatestVersion()
_, latestVersion, err := tree.ndb.getLatestVersion()
require.NoError(t, err)
if latestVersion != version {
// The fast node disk check should only be done to the latest version
Expand Down
Loading