Avoid usage of RocksDB deleteRange() in DbLedgerStorage #1620
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
sijie
approved these changes
Aug 23, 2018
eolivelli
approved these changes
Aug 23, 2018
|
run integration tests |
ivankelly
approved these changes
Aug 23, 2018
sijie
pushed a commit
that referenced
this pull request
Aug 23, 2018
### Motivation There are few issues that are reconducible to a performance degradation in RocksDB when using deleteRange() feature (eg: apache/pulsar#1737 and others). There is some discussion going on RocksDB to address this issue: facebook/rocksdb#3959 In the meantime, we should rollback the change and don't use deleteRange until these issues are resolved. ### Changes This PR is essentially reverting back the commit YahooArchive@4b84990 from Yahoo branch (which was squashed when merging back to apache). The only addition here is to use `DELETE_ENTRIES_BATCH_SIZE` to amortize the cost of `batch.flush()` when there are many ledgers with few entries. Author: Matteo Merli <mmerli@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #1620 from merlimat/rollback-delete-range (cherry picked from commit dca471d) Signed-off-by: Sijie Guo <sijie@apache.org>
sijie
pushed a commit
that referenced
this pull request
Aug 23, 2018
### Motivation There are few issues that are reconducible to a performance degradation in RocksDB when using deleteRange() feature (eg: apache/pulsar#1737 and others). There is some discussion going on RocksDB to address this issue: facebook/rocksdb#3959 In the meantime, we should rollback the change and don't use deleteRange until these issues are resolved. ### Changes This PR is essentially reverting back the commit YahooArchive@4b84990 from Yahoo branch (which was squashed when merging back to apache). The only addition here is to use `DELETE_ENTRIES_BATCH_SIZE` to amortize the cost of `batch.flush()` when there are many ledgers with few entries. Author: Matteo Merli <mmerli@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #1620 from merlimat/rollback-delete-range (cherry picked from commit dca471d) Signed-off-by: Sijie Guo <sijie@apache.org>
hangc0276
added a commit
that referenced
this pull request
Nov 18, 2022
…ance (#3653) ### Motivation The entry log location index deletion is deleted in batches one by one currently, and it will have low performance. Refer to: #3646 Matteo has introduced deleteRange API a few years ago, but rollback due to RocksDB delete ranges bug. #1620. The RocksDB bug has been addressed since 5.18.0 https://github.com/facebook/rocksdb/blob/main/HISTORY.md#5180-2018-11-30. We can bring the `deleteRange` API back to improve the entry log location deletion performance. ### Changes Bring `deleteRange` API back for entry log location deletion.
hangc0276
added a commit
that referenced
this pull request
Nov 21, 2022
…ance (#3653) The entry log location index deletion is deleted in batches one by one currently, and it will have low performance. Refer to: #3646 Matteo has introduced deleteRange API a few years ago, but rollback due to RocksDB delete ranges bug. #1620. The RocksDB bug has been addressed since 5.18.0 https://github.com/facebook/rocksdb/blob/main/HISTORY.md#5180-2018-11-30. We can bring the `deleteRange` API back to improve the entry log location deletion performance. Bring `deleteRange` API back for entry log location deletion. (cherry picked from commit 696919c)
zhaohaidao
pushed a commit
to zhaohaidao/bookkeeper
that referenced
this pull request
Nov 21, 2022
…ance (apache#3653) ### Motivation The entry log location index deletion is deleted in batches one by one currently, and it will have low performance. Refer to: apache#3646 Matteo has introduced deleteRange API a few years ago, but rollback due to RocksDB delete ranges bug. apache#1620. The RocksDB bug has been addressed since 5.18.0 https://github.com/facebook/rocksdb/blob/main/HISTORY.md#5180-2018-11-30. We can bring the `deleteRange` API back to improve the entry log location deletion performance. ### Changes Bring `deleteRange` API back for entry log location deletion.
nicoloboschi
pushed a commit
to datastax/bookkeeper
that referenced
this pull request
Jan 11, 2023
…ance (apache#3653) The entry log location index deletion is deleted in batches one by one currently, and it will have low performance. Refer to: apache#3646 Matteo has introduced deleteRange API a few years ago, but rollback due to RocksDB delete ranges bug. apache#1620. The RocksDB bug has been addressed since 5.18.0 https://github.com/facebook/rocksdb/blob/main/HISTORY.md#5180-2018-11-30. We can bring the `deleteRange` API back to improve the entry log location deletion performance. Bring `deleteRange` API back for entry log location deletion. (cherry picked from commit 696919c) (cherry picked from commit e159510)
yaalsn
pushed a commit
to yaalsn/bookkeeper
that referenced
this pull request
Jan 30, 2023
…ance (apache#3653) ### Motivation The entry log location index deletion is deleted in batches one by one currently, and it will have low performance. Refer to: apache#3646 Matteo has introduced deleteRange API a few years ago, but rollback due to RocksDB delete ranges bug. apache#1620. The RocksDB bug has been addressed since 5.18.0 https://github.com/facebook/rocksdb/blob/main/HISTORY.md#5180-2018-11-30. We can bring the `deleteRange` API back to improve the entry log location deletion performance. ### Changes Bring `deleteRange` API back for entry log location deletion.
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
There are few issues that are reconducible to a performance degradation in RocksDB when using deleteRange() feature (eg: apache/pulsar#1737 and others).
There is some discussion going on RocksDB to address this issue: facebook/rocksdb#3959
In the meantime, we should rollback the change and don't use deleteRange until these issues are resolved.
Changes
This PR is essentially reverting back the commit YahooArchive@4b84990 from Yahoo branch (which was squashed when merging back to apache).
The only addition here is to use
DELETE_ENTRIES_BATCH_SIZEto amortize the cost ofbatch.flush()when there are many ledgers with few entries.