feat: add the tree Close api #905
Conversation
WalkthroughThe recent update introduces a 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 (
|
|
backport to release/v1.1.x for store/v2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nodedb.go
Outdated
| // Close the nodeDB. | ||
| func (ndb *nodeDB) Close() error { | ||
| ndb.mtx.Lock() | ||
| defer ndb.mtx.Unlock() | ||
|
|
||
| // skip the db.Close() since it can be used by other trees | ||
| return ndb.batch.Close() | ||
| } |
There was a problem hiding this comment.
The implementation of the Close method in the nodeDB struct correctly acquires a lock to ensure thread safety and closes the batch. However, it's noted that db.Close() is intentionally skipped, which is a sensible decision if the database connection is shared across multiple instances or parts of the application. To improve code maintainability, consider adding a comment explaining the rationale behind not calling db.Close(), as this could be helpful for future maintainers or contributors who might wonder about this decision.
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.
| // Close the nodeDB. | |
| func (ndb *nodeDB) Close() error { | |
| ndb.mtx.Lock() | |
| defer ndb.mtx.Unlock() | |
| // skip the db.Close() since it can be used by other trees | |
| return ndb.batch.Close() | |
| } | |
| // Close the nodeDB. | |
| func (ndb *nodeDB) Close() error { | |
| ndb.mtx.Lock() | |
| defer ndb.mtx.Unlock() | |
| // skip the db.Close() since it can be used by other trees | |
| return ndb.batch.Close() | |
| } |
| func TestMutableTreeClose(t *testing.T) { | ||
| db := dbm.NewMemDB() | ||
| tree := NewMutableTree(db, 0, true, log.NewNopLogger()) | ||
|
|
||
| _, err := tree.Set([]byte("hello"), []byte("world")) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = tree.SaveVersion() | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, tree.Close()) |
There was a problem hiding this comment.
The test TestMutableTreeClose correctly tests the basic functionality of the Close method by setting a key-value pair, saving the version, and then closing the tree. However, it could be enhanced by adding assertions to verify the state of the tree after closing. For instance, attempting further operations on the closed tree (like setting a new key-value pair or saving another version) should result in an error. This would ensure that the Close method not only completes without errors but also effectively prevents further operations on the tree, which is crucial for resource management.
Consider adding the following checks:
- Verify that the internal state of the tree is as expected after closing (e.g., internal fields are set to
nil). - Attempt to perform operations on the tree after closing it and assert that these operations result in an error.
This would provide a more comprehensive test coverage for the Close method, ensuring it behaves as expected in various scenarios.
nodedb.go
Outdated
| defer ndb.mtx.Unlock() | ||
|
|
||
| // skip the db.Close() since it can be used by other trees | ||
| return ndb.batch.Close() |
There was a problem hiding this comment.
should we set ndb.batch = nil after close?
|
@Mergifyio backport release/v1.1.x |
✅ Backports have been created
|
(cherry picked from commit 0d441f5)
Co-authored-by: cool-developer <[email protected]>
Context
ref: cosmos/cosmos-sdk#19704