feat(store/v2): Support Custom WriteOpts PebbleDB #18195
Conversation
WalkthroughThe changes introduced provide more control over the synchronization behavior of batch writes in the PebbleDB storage system. A Changes
TipsChat with CodeRabbit Bot (
|
|
@alexanderbez your pull request is missing a changelog! |
github-prbot
requested review from
a team,
likhita-809 and
testinginprod
and removed request for
a team
![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: 0
Files selected for processing (4)
- store/storage/pebbledb/batch.go (3 hunks)
- store/storage/pebbledb/db.go (4 hunks)
- store/storage/pebbledb/db_test.go (1 hunks)
- store/storage/storage_bench_test.go (1 hunks)
Additional comments (Suppressed): 13
store/storage/pebbledb/db_test.go (2)
20-20: The
Newfunction now requires asyncparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of settingsynctofalsein this test. If the test is meant to simulate real-world usage, it might be more appropriate to setsynctotrue.32-32: The
Newfunction now requires asyncparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of settingsynctofalsein this test. If the test is meant to simulate real-world usage, it might be more appropriate to setsynctotrue.store/storage/storage_bench_test.go (1)
- 26-26: The
pebbledb.Newfunction now requires a second parameter to specify whether write operations should be synchronous or asynchronous. Ensure that this change does not affect the behavior of the benchmark tests, and that the choice of synchronous or asynchronous writes is appropriate for the tests.- return pebbledb.New(dataDir) + return pebbledb.New(dataDir, false)store/storage/pebbledb/batch.go (3)
22-25: The function signature of
NewBatchhas been changed to include asyncparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.33-36: The
Batchstruct now includes asyncfield. This is a good addition as it allows the caller to control whether write operations should be synchronous or asynchronous.69-72: The
Writemethod now uses thesyncfield to set theSyncoption when committing the batch. This is a good change as it allows the caller to control the synchronization behavior of write operations. However, please ensure that this change does not introduce any unexpected behavior in the rest of the codebase.store/storage/pebbledb/db.go (7)
24-24: The assertion that
Databaseimplementsstore.VersionedDatabaseis good practice to ensure that all required methods are implemented. This will cause a compile-time error if theDatabasestruct does not satisfy thestore.VersionedDatabaseinterface.26-39: The addition of the
syncfield to theDatabasestruct is a significant change. This field will control whether write operations are synchronous or asynchronous, which can have a major impact on the performance and durability of the database. The comments provide a clear explanation of the purpose and implications of this field.41-44: The
Newfunction now accepts asyncparameter, which is used to set thesyncfield in theDatabasestruct. This allows the caller to control the synchronization behavior of the database at the time of creation.54-54: The
syncfield is correctly set in theDatabasestruct using thesyncparameter passed to theNewfunction.58-62: The
NewWithDBfunction now also accepts asyncparameter, which is used to set thesyncfield in theDatabasestruct. This allows the caller to control the synchronization behavior of the database when creating aDatabaseinstance with an existing PebbleDB database.74-74: The
SetLatestVersionmethod now uses thesyncfield to set theSyncoption when writing to the database. This allows the synchronization behavior of the write operation to be controlled by thesyncfield in theDatabasestruct.144-144: The
ApplyChangesetmethod now passes thesyncfield to theNewBatchfunction. This allows the synchronization behavior of the batch to be controlled by thesyncfield in theDatabasestruct.
There was a problem hiding this comment.
Where would this get used? I dont think we should allow such a flag as it seems like it could cause other issues in shutdown
i do think there are other options we should look into configuring, The issue we had with current dbs was not looking deeper to get more performance. We should openan issue to test against our workloads and configure based on that. This change is based off small benchmarks which will show better results but its still not what we need for our use case
When the app instantiates the SS PebbleDB backend (assuming it's using PebbleDB).
I totally agree with you. I just wanted to give optionality. Sei, for example, would 100% use sync=false. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- store/storage/pebbledb/db.go (4 hunks)
- store/storage/pebbledb/db_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- store/storage/pebbledb/db_test.go
Additional comments: 5
store/storage/pebbledb/db.go (5)
24-38: The
syncfield has been added to theDatabasestruct. This field determines whether writes to the database are synchronous or asynchronous. The comments provide a clear explanation of the implications of setting this field to true or false. This is a good practice as it helps other developers understand the purpose and impact of this field.54-55: In the
Newfunction, thesyncfield is set totrueby default. This means that all writes to the database will be synchronous unless specified otherwise. This is a safe default setting as it ensures data durability.58-61: The
NewWithDBfunction now accepts asyncparameter. This allows the caller to specify whether writes to the database should be synchronous or asynchronous. This is a good addition as it provides more control over the database's write behavior.74-74: The
SetLatestVersionfunction now uses thesyncfield when setting the write options. This means that the write operation will be synchronous or asynchronous based on the value of thesyncfield. This is a good change as it allows the write behavior to be controlled on a per-database basis.144-144: The
ApplyChangesetfunction now passes thesyncfield to theNewBatchfunction. This means that the write behavior of the batch operation will be determined by the value of thesyncfield. This is a good change as it allows the write behavior to be controlled on a per-batch basis.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- store/storage/pebbledb/db.go (4 hunks)
- store/storage/pebbledb/db_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- store/storage/pebbledb/db_test.go
Additional comments: 5
store/storage/pebbledb/db.go (5)
24-38: The
syncfield has been added to theDatabasestruct to control the synchronization behavior of write operations. This is a significant change that can affect the durability and performance of write operations. Ensure that this change is well-documented and communicated to users of thepebbledbpackage.54-55: The
Newfunction now initializes thesyncfield totrueby default, meaning that write operations will be synchronous by default. This could potentially slow down write operations, but it will ensure their durability.58-61: The
NewWithDBfunction now accepts asyncparameter to control the synchronization behavior of write operations. This is a significant change that can affect the durability and performance of write operations. Ensure that this change is well-documented and communicated to users of thepebbledbpackage.74-74: The
SetLatestVersionmethod now uses thesyncfield to set theSyncoption in apebble.WriteOptionsstruct. This means that the synchronization behavior of theSetLatestVersionmethod will depend on the value of thesyncfield.144-144: The
ApplyChangesetmethod now accepts asyncparameter, which is passed to theNewBatchfunction. This means that the synchronization behavior of theApplyChangesetmethod will depend on the value of thesyncfield.
|
Hey @alexanderbez @tac0turtle can we please backport this to the |
|
Hi @GAtom22 -- these changes are in respect to the store v2 work which is yet unreleased. I.e. this code does not exist on v0.47.x. What specifically do you want to see on v0.47.x? |
Ahh, nvm, my bad |
Description
Closes: #18178
Turns out, the default PebbleDB opts are sufficiently fine and show no major difference when tuned. The write options on batch writes show significant improvement in writes.
This PR allows apps to set the sync (write options).
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
Please note, this update does not directly affect the user interface but enhances the underlying data handling capabilities of the software.