feat(store/v2): Add Pruning Tests & Fix SQLite & PebbleDB Pruning #18459
Conversation
## Walkthrough
The changes across various database implementations (PebbleDB, RocksDB, SQLite) focus on introducing and handling a new concept of pruning data. A `pruneHeightKey` is used to track the earliest version of data that should be retained, with older data being pruned. This involves adding checks against the `earliestVersion` to prevent access to pruned data, updating error messages to reflect pruned versions, and modifying tests to account for the new pruning behavior. The changes aim to improve database performance and manage storage more efficiently.
## Changes
| File Path | Change Summary |
|-----------|----------------|
| `store/storage/pebbledb/db.go` <br> `store/storage/rocksdb/db.go` <br> `store/storage/sqlite/db.go` | Introduced pruning logic, added `earliestVersion` field, and updated methods to handle pruned versions. |
| `store/storage/pebbledb/db_test.go` <br> `store/storage/sqlite/db_test.go` <br> `store/storage/storage_test_suite.go` | Updated tests to handle new synchronization settings and pruning logic, including changes to assertions and removal of time logging. |
| `store/storage/pebbledb/iterator.go` <br> `store/storage/sqlite/iterator.go` | Added checks for `earliestVersion` in iterators to invalidate them if they target pruned versions. |
| `store/storage/sqlite/batch.go` | Updated SQL execution call from `latestVersionStmt` to `reservedUpsertStmt`. |
| `store/errors.go` | Changed error message for code 11 to "version pruned" and introduced a new error type `ErrVersionPruned`. |
> In databases deep, changes take a leap,
> Pruning old data, errors to keep.
> As seasons turn and history unfolds,
> Efficiency and joy, the code beholds.TipsChat with CodeRabbit Bot (
|
alexanderbez
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (9)
- store/storage/pebbledb/db.go (11 hunks)
- store/storage/pebbledb/db_test.go (1 hunks)
- store/storage/pebbledb/iterator.go (1 hunks)
- store/storage/rocksdb/db.go (6 hunks)
- store/storage/sqlite/batch.go (1 hunks)
- store/storage/sqlite/db.go (9 hunks)
- store/storage/sqlite/db_test.go (4 hunks)
- store/storage/sqlite/iterator.go (3 hunks)
- store/storage/storage_test_suite.go (2 hunks)
Files skipped from review due to trivial changes (1)
- store/storage/sqlite/db_test.go
Additional comments: 32
store/storage/pebbledb/db_test.go (1)
- 12-25: The update to the
NewDBfunction within the test suite to disable synchronization is a common practice for speeding up tests that do not require the durability guarantees of disk synchronization. This change should be verified to ensure it does not affect any tests that rely on the synchronization behavior.store/storage/sqlite/batch.go (1)
- 67-71: The change from
latestVersionStmttoreservedUpsertStmtshould be verified to ensure that the SQL statement executed aligns with the intended logic for theWritemethod. This change could have implications on how thelatestVersionis managed within the database, potentially affecting the pruning logic and data integrity.Ensure that
reservedUpsertStmtis correctly defined elsewhere in the code and that it performs the expected operation. Also, verify thatreservedStoreKeyandkeyLatestHeightare properly defined and used.store/storage/sqlite/iterator.go (3)
25-32: The addition of a check for
targetVersionagainstdb.earliestVersionis a good practice to prevent the iterator from accessing pruned data. This ensures that the iterator respects the pruning logic and does not return invalid data.60-66: The SQL statement preparation is done correctly using placeholders to prevent SQL injection. This is a critical security measure to protect against injection attacks.
101-110: The
Closemethod has been updated to check ifitr.statementis not nil before attempting to close it. This is a good practice to avoid potential nil pointer dereferences.store/storage/storage_test_suite.go (1)
- 482-532: The test case
TestDatabase_Prune_KeepRecentis well-structured and covers the scenario of pruning while ensuring that data written after the prune point remains accessible. It's important to ensure that the pruning functionality is thoroughly tested across all database backends to prevent data loss or corruption. This test case seems to be a valuable addition to the test suite.store/storage/pebbledb/iterator.go (2)
32-43: The addition of the
earliestVersionparameter and the logic to return an invalid iterator if theversionis less thanearliestVersionis a good approach to handle attempts to iterate over pruned data. This change ensures that the iterator respects the pruning logic and does not return data that has been deleted.47-47: The handling of reverse iteration by using
src.Last()is correct, but it's important to note that the comment on lines 8-10 states that reverse iteration is not supported. If reverse iteration is now being supported, the comment should be updated to reflect this change. If not, then the logic for reverse iteration should be removed or commented out with an explanation.store/storage/rocksdb/db.go (5)
7-13: The import of the "strings" package is added to handle error messages. Ensure that this is the only place where string manipulation is required, and consider if there are any other string operations that could benefit from this import to justify its inclusion.
33-36: The removal of the
tsLowfield from theDatabasestruct simplifies the pruning logic by relying on error handling during read operations instead of manual tracking. This change should be cross-checked with all usages of theDatabasestruct to ensure that no other parts of the code rely on thetsLowfield.66-78: The
getSlicefunction has been updated to handle cases where a query for a pruned version does not return an error. This change assumes that the error message contains a specific string. It's important to ensure that this string matching is reliable and that the error message format will not change in future versions of the underlying database library, as this could lead to silent failures.102-121: The
HasandGetfunctions have been updated to remove the check forversion < db.tsLowand to handle cases where a query for a pruned version does not return an error. This change is consistent with the removal oftsLowand the new error handling strategy. Ensure that all error paths are correctly handled and that the absence of an error correctly indicates the absence of a value due to pruning.156-160: The
Prunefunction has been updated to remove the update ofdb.tsLow. This change is consistent with the removal of thetsLowfield and the new pruning strategy. Ensure that the new pruning logic is thoroughly tested, especially since it relies on the behavior of the underlying database library to handle pruned versions correctly.store/storage/sqlite/db.go (7)
18-26: The addition of
keyPruneHeightconstant is consistent with the existing pattern of defining key constants. This will be used to track the pruning height within the database.69-87: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [44-85]
The
Databasestruct has been updated to include anearliestVersionfield, which is initialized in theNewfunction using thegetPruneHeighthelper function. This is a critical change for the pruning logic, ensuring that the database knows the earliest version it can serve. TheearliestVersionis set topruneHeight + 1, which assumes thatpruneHeightis the last pruned version and the database should serve versions greater than this.
113-119: The
SetLatestVersionmethod has been updated to usereservedUpsertStmtfor SQL execution. This change is consistent with the pattern of using prepared statements for database operations, which can help prevent SQL injection attacks and improve performance.154-163: The
Getmethod now includes a check againstdb.earliestVersionto ensure that it does not return data that has been pruned. This is a critical addition for the correctness of the pruning functionality.236-240: The
Iteratormethod's implementation looks correct. It checks for invalid start and end ranges and creates a new iterator with the provided parameters.248-252: The
ReverseIteratormethod mirrors theIteratormethod but specifies that the iterator should be in reverse order. The checks and the creation of the iterator are consistent with the forward iterator.285-307: The
getPruneHeightfunction is a new utility function to retrieve the prune height from the database. It uses a prepared statement to query thekeyPruneHeightfrom thestate_storagetable. The error handling is consistent with the rest of the codebase, including handlingsql.ErrNoRowsto indicate a fresh database with no prune height set yet.store/storage/pebbledb/db.go (12)
16-26: The constants and global variables are well-defined and follow the naming conventions. However, ensure that the comments are accurate and that the lexical ordering note is indeed correct and necessary for the system's logic.
30-39: The
Databasestruct is updated to include theearliestVersionfield, which is essential for the pruning logic. The comment provides a good explanation of thesyncfield's purpose. This change seems appropriate for the new functionality.57-69: The error handling in the
Newfunction is correct, and the use offmt.Errorfto wrap the underlying error is a good practice for error tracing. The retrieval of thepruneHeightand setting of theearliestVersiontopruneHeight + 1is logical, assuming thatpruneHeightrepresents the last pruned version.95-100: The
SetLatestVersionmethod is concise and correct. It uses little-endian encoding to store the version number, which is consistent with the rest of the codebase.117-127: The
setPruneHeightmethod correctly updates theearliestVersionand persists thepruneVersionusing little-endian encoding. The use ofdb.syncin the write options ensures that the sync behavior is consistent with the rest of the database operations.138-145: The
Getmethod now includes a check to ensure that thetargetVersionis not less than theearliestVersion, which is a crucial addition for the pruning logic. The error handling and the return ofnilwhen thetargetVersionis pruned are correct.164-168: The error handling for decoding the tombstone is correct. The comment explaining the tombstone logic is clear and helps in understanding the code's intent.
196-277: The
Prunemethod has been significantly updated. The batch deletion logic and the use ofPruneCommitBatchSizefor committing in batches are good for performance. The error handling and the use ofdb.syncfor commit operations are consistent with the rest of the database operations. The final call todb.setPruneHeight(version)ensures that theearliestVersionis updated after pruning.297-300: The
Iteratormethod correctly creates a new iterator with the given bounds and version constraints. The use ofdb.earliestVersionensures that the iterator respects the pruning logic.321-324: The
ReverseIteratormethod mirrors theIteratormethod in terms of logic and error handling, which is consistent and correct.332-372: The utility functions
storePrefix,prependStoreKey,getPruneHeight, andvalTombstonedare correctly implemented. ThevalTombstonedfunction includes a panic for an invalid MVCC value, which is a strong choice for a critical error that should not occur under normal circumstances. However, ensure that this panic will not cause undue service disruption and that it is handled or logged appropriately in the broader system context.375-376: The increment of
versionby 1 to make the end domain exclusive is correct. This is a common pattern in range queries to include the start key and exclude the end key.
| return New(dir) | ||
| db, err := New(dir) | ||
| if err == nil && db != nil { | ||
| db.SetSync(false) |
There was a problem hiding this comment.
in most environments this would be true, is there a reason to setting this to false for testing?
There was a problem hiding this comment.
Yes, just to make the unit tests faster. Not a strong reason, but it helps speed up CI testing time. (11.994s vs 1.214s)
There was a problem hiding this comment.
amazing!! could you add a short comment saying this. Would help for future readers
There was a problem hiding this comment.
a bit unrelated to this pr, but for pebbledb and sqllite it could make sense to have a current state table so we can get, set and iterate faster for current state as state grows on disk.
for sqllite we could do something like each module has its own table for current state and archived data is a single table. Somewhat different than iavlv2 but in the same vein
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- store/errors.go (1 hunks)
- store/storage/pebbledb/db.go (11 hunks)
- store/storage/rocksdb/db.go (6 hunks)
- store/storage/sqlite/db.go (9 hunks)
- store/storage/sqlite/db_test.go (5 hunks)
- store/storage/storage_test_suite.go (3 hunks)
Files skipped from review due to trivial changes (2)
- store/errors.go
- store/storage/sqlite/db_test.go
Additional comments: 15
store/storage/storage_test_suite.go (4)
192-195: The update to the
TestDatabase_IteratorClosetest function is good as it now checks for the expected behavior after the iterator is closed, which is thatiter.Valid()should returnfalse. This is a more appropriate test than checking for a panic, as the iterator should be designed to handle closure gracefully.456-466: The changes in the
TestDatabase_Prunefunction improve the test's robustness by checking that after pruning, attempts to access versions that have been pruned result in an error and that the data is no longer accessible. This aligns with the expected behavior of the pruning process.478-483: The continuation of the
TestDatabase_Prunefunction further asserts that after pruning the latest version, all data should be inaccessible, which is the expected behavior when the entire dataset is pruned.487-533: The addition of the
TestDatabase_Prune_KeepRecentfunction is a valuable test case that ensures pruning does not affect data versions that are newer than the pruned version. It checks that data before the prune point is inaccessible, while data after the prune point remains accessible. This is a critical test for ensuring the integrity of the pruning process.store/storage/rocksdb/db.go (4)
7-13: The addition of the "strings" import is necessary for the new error handling logic in the
getSlicefunction. This change is appropriate if the rest of the codebase does not use "strings" in this file.41-56: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [33-54]
The removal of the
tsLowfield from theDatabasestruct and its associated logic in theNewandNewWithDBfunctions aligns with the summary stating that manual tracking oftsLowis eliminated. This should simplify the codebase and reduce error rates as long as all other references totsLowhave been removed and the new error checking logic is robust.
100-119: The
HasandGetfunctions have been updated to remove the check forversion < db.tsLowand to handle the potentialstore.ErrVersionPrunederror fromgetSlice. This change is consistent with the removal oftsLowand the new error handling strategy. Ensure that all callers of these functions are updated to handle the new error appropriately.154-158: The
Prunefunction has been updated to useIncreaseFullHistoryTsLowinstead of manually updatingtsLow. This aligns with the summary's point about shifting the RocksDB pruning approach. Ensure that this change is tested thoroughly since it affects how data is pruned and could have implications for data integrity.store/storage/sqlite/db.go (7)
18-26: The
keyPruneHeightconstant is introduced correctly and thereservedUpsertStmtis updated to handle upserts for reserved keys. This looks good as long as the rest of the codebase is updated to handle the newkeyPruneHeightconstant where necessary.69-87: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [45-85]
The
Databasestruct now includes anearliestVersionfield, and theNewfunction initializes this field based on the retrieved prune height. This is a good approach to track the oldest available version after pruning. Ensure that theearliestVersionis correctly used throughout the codebase.
113-119: The
SetLatestVersionmethod correctly updates the latest version in the database. Ensure that this method is called appropriately whenever a new version is committed to the database.131-137: The
Getmethod now includes a check to ensure that the requested version is not earlier than theearliestVersion. This is a good security measure to prevent access to pruned data. However, ensure that the error handling forstore.ErrVersionPrunedis implemented wherever this method is called.239-243: The
Iteratormethod correctly checks for invalid start and end ranges and returns an error if the start is after the end. This is a good practice to prevent unexpected behavior during iteration.251-255: The
ReverseIteratormethod has the same checks as theIteratormethod, which is consistent and correct.288-310: The
getPruneHeightfunction is implemented to retrieve the prune height from the database. It correctly handles the case where no rows are found, returning zero in that case. This is a good approach to initialize theearliestVersionwhen the database is new or has not been pruned yet.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- store/errors.go (2 hunks)
- store/storage/pebbledb/db.go (11 hunks)
- store/storage/rocksdb/db.go (5 hunks)
- store/storage/sqlite/db.go (9 hunks)
Additional comments: 26
store/errors.go (2)
3-7: The import of the "fmt" package is correctly added to support the new error formatting in
ErrVersionPruned. Ensure that the "cosmossdk.io/errors" package is the correct one for registering errors in this context.47-48: The
Error()method forErrVersionPrunedis correctly implemented, providing a formatted error message that includes theEarliestVersionfield. This is a good use of Go's error handling pattern.store/storage/rocksdb/db.go (5)
33-39: The comment on
tsLowhas been updated to reflect its purpose more clearly. This is a good practice as it helps maintainers understand the significance of the variable within the context of the code.73-79: The
NewWithDBfunction has been updated to initializetsLowonly iftsLowBzis not empty. This is a logical change that prevents potential misuse of an uninitialized variable.91-100: The
getSlicemethod now includes a check forversion < db.tsLowand returns astore.ErrVersionPrunederror if the condition is true. This is an important change for correctness, ensuring that attempts to access pruned versions are handled properly.126-132: The
Hasmethod has been updated to handle the case wheresliceisnil. This is a defensive programming practice that helps prevent potential nil pointer dereferences.180-184: The
Prunemethod has been updated to incrementtsLowby 1 to include the provided version in the pruning process. This is a logical change that aligns with the intended functionality of pruning up to and including the specified version.store/storage/sqlite/db.go (5)
18-26: The definition of
keyPruneHeightconstant and the SQL statement forreservedUpsertStmtare correctly implemented. The SQL statement uses anINSERT ... ON CONFLICT ... DO UPDATEpattern which is appropriate for upsert operations in SQLite.69-87: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [44-85]
The
Databasestruct has been updated to include anearliestVersionfield, and theNewfunction has been modified to initialize this field with the value ofpruneHeight + 1. This is a logical change assuming thatpruneHeightrepresents the last pruned version, and the earliest available version in the database should be the next one. However, ensure that thegetPruneHeightfunction correctly handles the case where there is no prune height set in a fresh database.
113-119: The
SetLatestVersionmethod correctly updates thelatest_heightin the database. The use ofreservedUpsertStmtensures that the value is inserted or updated if it already exists.131-137: The
Getmethod now includes a check againstdb.earliestVersionto ensure that the requested version has not been pruned. This is a good practice for preventing access to data that should no longer be available. The error returned is specific and informative.239-245: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [239-255]
The
IteratorandReverseIteratormethods are correctly implemented with checks for empty keys and start-after-end errors. These checks are important for ensuring that the iterators behave as expected and do not return invalid ranges of data.store/storage/pebbledb/db.go (14)
16-26: The constants are well-defined and follow the Go naming conventions. The comments provide useful information about the purpose of each constant.
30-39: The
Databasestruct is well-defined and includes a new fieldearliestVersion. The comment explains its purpose clearly. However, ensure that all usages of theDatabasestruct throughout the codebase are updated to handle this new field appropriately.57-69: The error handling in the
Newfunction is good, wrapping the underlying error with additional context. However, thepruneHeight + 1logic assumes thatgetPruneHeightreturns the last pruned height, not the earliest available version. Ensure this assumption is correct and documented.95-100: The
SetLatestVersionmethod is concise and uses thebinarypackage to encode the version number, which is a standard approach in Go for binary encoding.117-118: The
GetLatestVersionmethod correctly handles the case where the latest version key might not be found, which is a good practice for handling optional data.120-126: The
setPruneHeightmethod updates theearliestVersionfield and persists the prune height to the database. Ensure that this method is called in the correct context where the pruning operation is guaranteed to have completed successfully.138-141: The
Getmethod now includes a check againstearliestVersionto ensure that the requested version has not been pruned. This is a good practice for data integrity and user feedback.164-167: The error handling here is consistent and provides clear messaging about the nature of the error, which is good for maintainability and debugging.
196-277: The
Prunemethod has been significantly updated. The comments provide a clear warning about the potential inefficiency of the method. The batch processing logic is a good practice to handle large numbers of deletions without overwhelming the database transaction log. However, ensure that the batch size defined byPruneCommitBatchSizeis tuned based on the database performance characteristics and the expected workload.297-300: The
Iteratormethod creates a new iterator with the correct bounds and passes theearliestVersionto thenewPebbleDBIteratorfunction. This ensures that the iterator respects the pruning logic.321-324: The
ReverseIteratormethod mirrors theIteratormethod but is intended for reverse iteration. The same considerations apply here as for theIteratormethod.332-333: The
prependStoreKeyfunction is a utility that correctly appends the store key prefix to a given key. This is a common pattern in key-value stores to namespace keys.335-350: The
getPruneHeightfunction retrieves the prune height from the database and handles the case where the prune height key might not exist, which is good for robustness.374-376: The
getMVCCSlicefunction correctly increments the version to make the end domain exclusive. Ensure that this logic is well-documented and understood by the callers of this function.
Good thinking, but it wouldn't help. For PebbleDB, you would need a separate DB entirely which complicates the codebase significantly. And basic benchmarks show it's on par with RocksDB (not as fast as expected). For SQLite, we would need a separate table. But again, complicating the codebase. I'd like to see Osmosis numbers before going such a route :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- store/errors.go (2 hunks)
- store/storage/rocksdb/db.go (5 hunks)
Additional comments: 7
store/errors.go (2)
3-7: The import of
"cosmossdk.io/errors"should be verified to ensure it is the correct package and that it is being used elsewhere in the code. If this import is not used, it should be removed to keep the code clean.34-49: The addition of
ErrVersionPrunedis a good practice for handling specific error cases related to version pruning. It is important to ensure that this error is handled wherever versions are queried in the codebase. The error message is clear and informative, providing the earliest available version when a pruned version is requested.store/storage/rocksdb/db.go (5)
33-39: The
Databasestruct has been updated to include atsLowfield, which represents the earliest version supported by the database. This is a significant change as it introduces a new constraint on the data retrieval methods to check against thistsLowvalue to prevent access to pruned data.73-79: The
NewWithDBfunction has been correctly updated to initialize thetsLowfield of theDatabasestruct. This ensures that thetsLowvalue is set when a newDatabaseinstance is created with an existing RocksDB instance and column family handle.91-100: The
getSlicemethod now includes a check to ensure that the requested version is not less thantsLow. If it is, anErrVersionPrunederror is returned. This is a critical update for maintaining data integrity and ensuring that clients do not attempt to read data that has been pruned.126-129: The
Hasmethod has been updated to use the newgetSlicemethod, which includes thetsLowcheck. This ensures that theHasmethod respects the pruning logic and does not return true for keys that belong to pruned versions.135-138: The
Getmethod has also been updated to use thegetSlicemethod. The error message has been enhanced to provide more context by wrapping the original error with a message indicating the failure to get a RocksDB slice. This improves error handling by providing more detailed information about the nature of the error.
…k into bez/fix-sqlite-ss-prune
| t2.store_key = state_storage.store_key AND | ||
| t2.key = state_storage.key AND | ||
| t2.version <= ? | ||
| ) AND store_key != ?; |
There was a problem hiding this comment.
new table (max_version, key, store_key) could be helpful? this query seems like very heavy
store/storage/pebbledb/db_test.go
Outdated
| func TestStorageTestSuite(t *testing.T) { | ||
| s := &storage.StorageTestSuite{ | ||
| NewDB: func(dir string) (store.VersionedDatabase, error) { | ||
| return New(dir) | ||
| db, err := New(dir) | ||
| if err == nil && db != nil { | ||
| // we set sync=false just to speed up CI tests | ||
| db.SetSync(false) | ||
| } | ||
|
|
||
| return db, err | ||
| }, | ||
| EmptyBatchSize: 12, | ||
| SkipTests: []string{ | ||
| "TestStorageTestSuite/TestDatabase_Prune", | ||
| }, | ||
| } | ||
|
|
||
| suite.Run(t, s) |
There was a problem hiding this comment.
The change to set the database sync behavior to false for speeding up CI tests is reasonable, but it should be documented in the code as per the discussion in the comments. A comment should be added to explain why db.SetSync(false) is being called, to provide context for future maintainers.
if err == nil && db != nil {
+ // Set sync to false to speed up CI tests (11.994s vs 1.214s)
db.SetSync(false)
}Commitable 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 TestStorageTestSuite(t *testing.T) { | |
| s := &storage.StorageTestSuite{ | |
| NewDB: func(dir string) (store.VersionedDatabase, error) { | |
| return New(dir) | |
| db, err := New(dir) | |
| if err == nil && db != nil { | |
| // we set sync=false just to speed up CI tests | |
| db.SetSync(false) | |
| } | |
| return db, err | |
| }, | |
| EmptyBatchSize: 12, | |
| SkipTests: []string{ | |
| "TestStorageTestSuite/TestDatabase_Prune", | |
| }, | |
| } | |
| suite.Run(t, s) | |
| func TestStorageTestSuite(t *testing.T) { | |
| s := &storage.StorageTestSuite{ | |
| NewDB: func(dir string) (store.VersionedDatabase, error) { | |
| db, err := New(dir) | |
| if err == nil && db != nil { | |
| // Set sync to false to speed up CI tests (11.994s vs 1.214s) | |
| db.SetSync(false) | |
| } | |
| return db, err | |
| }, | |
| EmptyBatchSize: 12, | |
| } | |
| suite.Run(t, s) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/storage/pebbledb/db_test.go (1 hunks)
Additional comments: 1
store/storage/pebbledb/db_test.go (1)
- 12-27: The changes here reflect the addition of a comment explaining the rationale behind setting
db.SetSync(false), which is to speed up CI tests. This is a good practice as it provides context for future maintainers of the code. The removal of theSkipTestsfield is not shown in the hunk, but if it was removed, ensure that it is not used elsewhere in the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/pruning/manager_test.go (4 hunks)
Additional comments: 4
store/pruning/manager_test.go (4)
44-51: The test function
TestPruningis being skipped entirely withs.T().SkipNow(). This should be verified to ensure it is intentional, as it means the test will not run and therefore will not validate any pruning logic.55-70: The loop correctly writes batches and commits them for each version, then applies the changeset and prunes the version. The error handling after each operation is appropriate, ensuring that the test fails fast if an operation does not succeed as expected.
77-83: The test checks that the value for "key" at
latestVersion-4(which should be version 96) is as expected, and that the value at version 50 is nil, which implies it has been pruned. This is a good check to ensure that pruning is working as intended. However, it's important to ensure that the pruning settings (set in lines 49-50) are such that version 50 should indeed be pruned at this point in the test.87-93: The test checks for the existence of a proof for "key" at
latestVersion-4and the absence of a proof atlatestVersion-5. This is consistent with the pruning settings and the expected behavior of the system. However, the test should ensure that the error received when getting the proof forlatestVersion-5is specifically due to the version being pruned (e.g.,ErrVersionPruned) and not some other error.
Description
closes: #18462
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Testing
Documentation