Avoid compaction to trigger extra flushes DbLedgerStorage #3959
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@merlimat There is one test that failed related to compaction, please help take a look, thanks. |
dlg99
approved these changes
May 22, 2023
wenbingshen
approved these changes
May 22, 2023
This was referenced May 30, 2023
hangc0276
pushed a commit
to streamnative/bookkeeper-achieved
that referenced
this pull request
Jun 7, 2023
* Avoid compaction to trigger extra flushes DbLedgerStorage * Expanded comment * Fixed test * Fixed direct io test (cherry picked from commit c924cfe)
zymap
pushed a commit
that referenced
this pull request
Jun 19, 2023
* Avoid compaction to trigger extra flushes DbLedgerStorage * Expanded comment * Fixed test * Fixed direct io test (cherry picked from commit c924cfe)
hangc0276
pushed a commit
to hangc0276/bookkeeper
that referenced
this pull request
Jun 26, 2023
* Avoid compaction to trigger extra flushes DbLedgerStorage * Expanded comment * Fixed test * Fixed direct io test (cherry picked from commit c924cfe)
Merged
zymap
pushed a commit
that referenced
this pull request
Dec 6, 2023
* Avoid compaction to trigger extra flushes DbLedgerStorage * Expanded comment * Fixed test * Fixed direct io test (cherry picked from commit c924cfe)
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
* Avoid compaction to trigger extra flushes DbLedgerStorage * Expanded comment * Fixed test * Fixed direct io test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR solves the problem described #3940.
Because compaction is triggering a flush after each entry log file is compacted, the flush on the DbLedgerStorage causes the write cache to be underutilized and ultimately it will slow down the writes.
Changes
The call to
flush()inupdateEntriesLocations()was there to ensure that every entry log file we might be compacting has the indexes already flushed into RocksDB. If that's not the case, the compaction might first write the new location and the regular flush will override it with the old location, to a file that is already deleted.In practice, we don't actually need to trigger a new flush for that to be valid, we just need to ensure that if there is a pending
flush(), we wait until it's done. If there are no pending flushes, we're also good to go.