fix: the reference root issue #909
Conversation
WalkthroughThe recent updates focus on enhancing consistency, clarity, and efficiency in handling node storage and version management. Key modifications include method renaming for uniformity, improved commentary for easier understanding, and a refined approach to dealing with legacy nodes. Additionally, the logic for deleting versions has been clarified and optimized, with particular attention to batch deletions and mutex handling for safer concurrency operations. These changes aim to streamline operations and maintenance of the data structure. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func TestEmptyVersionDelete(t *testing.T) { | ||
| db, err := dbm.NewDB("test", "memdb", "") | ||
| require.NoError(t, err) | ||
| defer db.Close() | ||
|
|
||
| tree := NewMutableTree(db, 0, false, log.NewNopLogger()) | ||
|
|
||
| _, err = tree.Set([]byte("key1"), []byte("value1")) | ||
| require.NoError(t, err) | ||
|
|
||
| toVersion := 10 | ||
| for i := 0; i < toVersion; i++ { | ||
| _, _, err = tree.SaveVersion() | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| require.NoError(t, tree.DeleteVersionsTo(5)) | ||
|
|
||
| // Load the tree from disk. | ||
| tree = NewMutableTree(db, 0, false, log.NewNopLogger()) | ||
| v, err := tree.Load() | ||
| require.NoError(t, err) | ||
| require.Equal(t, int64(toVersion), v) | ||
| // Version 1 is only meaningful, so it should not be deleted. | ||
| require.Equal(t, tree.root.GetKey(), (&NodeKey{version: 1, nonce: 0}).GetKey()) | ||
| // it is expected that the version 1 is deleted. | ||
| versions := tree.AvailableVersions() | ||
| require.Equal(t, 6, versions[0]) | ||
| require.Len(t, versions, 5) | ||
| } |
There was a problem hiding this comment.
The test TestEmptyVersionDelete aims to verify the behavior of deleting versions up to a certain point and ensuring that the tree loads correctly afterward. However, there's a discrepancy in the comments and the expected behavior:
- Line 1849 suggests that version 1 should not be deleted, which aligns with the assertion checking the root key for version 1.
- Line 1850, however, comments that it is expected version 1 is deleted, which contradicts the previous assertion and the actual test logic.
To clarify the test's intention and ensure it accurately reflects the expected behavior, please revise the comments to consistently describe the expected outcome of the test.
- // it is expected that the version 1 is deleted.
+ // it is expected that versions up to 5 are deleted, leaving versions 6 and above.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.
| func TestEmptyVersionDelete(t *testing.T) { | |
| db, err := dbm.NewDB("test", "memdb", "") | |
| require.NoError(t, err) | |
| defer db.Close() | |
| tree := NewMutableTree(db, 0, false, log.NewNopLogger()) | |
| _, err = tree.Set([]byte("key1"), []byte("value1")) | |
| require.NoError(t, err) | |
| toVersion := 10 | |
| for i := 0; i < toVersion; i++ { | |
| _, _, err = tree.SaveVersion() | |
| require.NoError(t, err) | |
| } | |
| require.NoError(t, tree.DeleteVersionsTo(5)) | |
| // Load the tree from disk. | |
| tree = NewMutableTree(db, 0, false, log.NewNopLogger()) | |
| v, err := tree.Load() | |
| require.NoError(t, err) | |
| require.Equal(t, int64(toVersion), v) | |
| // Version 1 is only meaningful, so it should not be deleted. | |
| require.Equal(t, tree.root.GetKey(), (&NodeKey{version: 1, nonce: 0}).GetKey()) | |
| // it is expected that the version 1 is deleted. | |
| versions := tree.AvailableVersions() | |
| require.Equal(t, 6, versions[0]) | |
| require.Len(t, versions, 5) | |
| } | |
| func TestEmptyVersionDelete(t *testing.T) { | |
| db, err := dbm.NewDB("test", "memdb", "") | |
| require.NoError(t, err) | |
| defer db.Close() | |
| tree := NewMutableTree(db, 0, false, log.NewNopLogger()) | |
| _, err = tree.Set([]byte("key1"), []byte("value1")) | |
| require.NoError(t, err) | |
| toVersion := 10 | |
| for i := 0; i < toVersion; i++ { | |
| _, _, err = tree.SaveVersion() | |
| require.NoError(t, err) | |
| } | |
| require.NoError(t, tree.DeleteVersionsTo(5)) | |
| // Load the tree from disk. | |
| tree = NewMutableTree(db, 0, false, log.NewNopLogger()) | |
| v, err := tree.Load() | |
| require.NoError(t, err) | |
| require.Equal(t, int64(toVersion), v) | |
| // Version 1 is only meaningful, so it should not be deleted. | |
| require.Equal(t, tree.root.GetKey(), (&NodeKey{version: 1, nonce: 0}).GetKey()) | |
| // it is expected that versions up to 5 are deleted, leaving versions 6 and above. | |
| versions := tree.AvailableVersions() | |
| require.Equal(t, 6, versions[0]) | |
| require.Len(t, versions, 5) | |
| } |
| // SetFastStorageVersionToBatch sets storage version to fast where the version is | ||
| // 1.1.0-<version of the current live state>. Returns error if storage version is incorrect or on | ||
| // db error, nil otherwise. Requires changes to be committed after to be persisted. | ||
| func (ndb *nodeDB) setFastStorageVersionToBatch(latestVersion int64) error { | ||
| func (ndb *nodeDB) SetFastStorageVersionToBatch(latestVersion int64) error { | ||
| ndb.mtx.Lock() | ||
| defer ndb.mtx.Unlock() | ||
|
|
There was a problem hiding this comment.
The method SetFastStorageVersionToBatch now includes a locking mechanism to ensure thread safety. However, the comparison ndb.storageVersion >= fastStorageVersionValue uses string comparison for version checking, which might not work as intended for semantic versioning. Consider implementing or using a library for semantic version comparison to avoid potential issues.
- if ndb.storageVersion >= fastStorageVersionValue {
+ if compareSemanticVersions(ndb.storageVersion, fastStorageVersionValue) >= 0 {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.
| // SetFastStorageVersionToBatch sets storage version to fast where the version is | |
| // 1.1.0-<version of the current live state>. Returns error if storage version is incorrect or on | |
| // db error, nil otherwise. Requires changes to be committed after to be persisted. | |
| func (ndb *nodeDB) setFastStorageVersionToBatch(latestVersion int64) error { | |
| func (ndb *nodeDB) SetFastStorageVersionToBatch(latestVersion int64) error { | |
| ndb.mtx.Lock() | |
| defer ndb.mtx.Unlock() | |
| // SetFastStorageVersionToBatch sets storage version to fast where the version is | |
| // 1.1.0-<version of the current live state>. Returns error if storage version is incorrect or on | |
| // db error, nil otherwise. Requires changes to be committed after to be persisted. | |
| func (ndb *nodeDB) SetFastStorageVersionToBatch(latestVersion int64) error { | |
| ndb.mtx.Lock() | |
| defer ndb.mtx.Unlock() | |
Implement async pruning of legacy nodes
Fix
When the root refers to the legacy root, it will reformat the legacy node as a new node key format. This node is removed while pruning but it is still being referred by the forward version.
Another issue with the reference node, it can refer to the internal node, not only the root. For example, if there are no adds, just removes, then the leaf or inner node can be the root.
nodeKeyinstead of only the version as a reference node valuenodeKeyto distinguish with the root. It will also be applied for the case of when pruning the version, reformat the given root nodeKey to (version, 0).