Make RrocksDB checksum type configurable #3793
Merged
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
hangc0276
requested review from
zymap,
eolivelli,
StevenLuMT,
dlg99 and
merlimat
Codecov Report
@@ Coverage Diff @@
## master #3793 +/- ##
============================================
+ Coverage 60.40% 68.25% +7.85%
- Complexity 5856 6751 +895
============================================
Files 473 473
Lines 40906 40910 +4
Branches 5232 5232
============================================
+ Hits 24711 27925 +3214
+ Misses 13981 10727 -3254
- Partials 2214 2258 +44
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
merlimat
approved these changes
Feb 21, 2023
zymap
approved these changes
Feb 22, 2023
horizonzy
reviewed
Feb 22, 2023
| @Test | ||
| public void testReadChecksumTypeFromBookieConfiguration() throws Exception { | ||
| ServerConfiguration configuration = new ServerConfiguration(); | ||
| configuration.setProperty("dbStorage_rocksDB_checksum_type", "kxxHash"); |
There was a problem hiding this comment.
We can remove configuration.setProperty("dbStorage_rocksDB_checksum_type", "kxxHash"); to verify the default value is kxxHash.
hangc0276
force-pushed
the
chenhang/make_rocksdb_checksum_type_configurable
branch
from
558ae30 to
830d57f
Compare
2 tasks
dlg99
pushed a commit
to datastax/bookkeeper
that referenced
this pull request
Jun 28, 2024
### Motivation Fix apache#3734 (comment) We have two rocksDB tables, one for the ledger index, and another for the entry log location. - ledger index RocksDB table: Use the default table option, and the checksum is `kCRC32c` - entry log location RocksDb table: Use configured table option, and the checksum is `kxxHash` When we upgrade the RocksDB version from 6.10.2 to 7.9.2, the new RocksDB version's default table checksum has changed from `kCRC32c` to `kXXH3`, and `kXXH3` only supported since RocksDB 6.27. The RocksDB version rollback to 6.10.2 will be failed due to RocksDB 6.10.2 doesn't support the `kXXH3` checksum type. ### Modifications In this PR, I make the RocksDB checksum type configurable. But there is one change that will change the ledger index RocksDB table's checksum type from the default `kCRC32c` to `kxxHash`. I have tested the compatibility of the two checksum types in and between multiple RocksDB versions, it works fine. After setting the two RocksDB table's checksum type to `kxxHash`, the RocksDB's version upgraded from 6.10.2 to 7.9.2, and rolling back to 6.10.2 works fine. ### More to discuss When writing the unit test to read the table checksum type from RocksDB configuration files, it failed. I found the related issue on RocksDB: facebook/rocksdb#5297 The related PR: facebook/rocksdb#10826 It means we still can't load RocksDB table options from configuration files. Maybe I missed some parts about reading RocksDB table options from the configuration file. If this issue exists, we do **NOT** recommend users configure RocksDB configurations through configuration files. @merlimat @eolivelli @dlg99 Please help take a look, thanks. (cherry picked from commit 3844bf1)
dlg99
pushed a commit
to datastax/bookkeeper
that referenced
this pull request
Jul 2, 2024
### Motivation Fix apache#3734 (comment) We have two rocksDB tables, one for the ledger index, and another for the entry log location. - ledger index RocksDB table: Use the default table option, and the checksum is `kCRC32c` - entry log location RocksDb table: Use configured table option, and the checksum is `kxxHash` When we upgrade the RocksDB version from 6.10.2 to 7.9.2, the new RocksDB version's default table checksum has changed from `kCRC32c` to `kXXH3`, and `kXXH3` only supported since RocksDB 6.27. The RocksDB version rollback to 6.10.2 will be failed due to RocksDB 6.10.2 doesn't support the `kXXH3` checksum type. ### Modifications In this PR, I make the RocksDB checksum type configurable. But there is one change that will change the ledger index RocksDB table's checksum type from the default `kCRC32c` to `kxxHash`. I have tested the compatibility of the two checksum types in and between multiple RocksDB versions, it works fine. After setting the two RocksDB table's checksum type to `kxxHash`, the RocksDB's version upgraded from 6.10.2 to 7.9.2, and rolling back to 6.10.2 works fine. ### More to discuss When writing the unit test to read the table checksum type from RocksDB configuration files, it failed. I found the related issue on RocksDB: facebook/rocksdb#5297 The related PR: facebook/rocksdb#10826 It means we still can't load RocksDB table options from configuration files. Maybe I missed some parts about reading RocksDB table options from the configuration file. If this issue exists, we do **NOT** recommend users configure RocksDB configurations through configuration files. @merlimat @eolivelli @dlg99 Please help take a look, thanks. (cherry picked from commit 3844bf1)
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
### Motivation Fix apache#3734 (comment) We have two rocksDB tables, one for the ledger index, and another for the entry log location. - ledger index RocksDB table: Use the default table option, and the checksum is `kCRC32c` - entry log location RocksDb table: Use configured table option, and the checksum is `kxxHash` When we upgrade the RocksDB version from 6.10.2 to 7.9.2, the new RocksDB version's default table checksum has changed from `kCRC32c` to `kXXH3`, and `kXXH3` only supported since RocksDB 6.27. The RocksDB version rollback to 6.10.2 will be failed due to RocksDB 6.10.2 doesn't support the `kXXH3` checksum type. ### Modifications In this PR, I make the RocksDB checksum type configurable. But there is one change that will change the ledger index RocksDB table's checksum type from the default `kCRC32c` to `kxxHash`. I have tested the compatibility of the two checksum types in and between multiple RocksDB versions, it works fine. After setting the two RocksDB table's checksum type to `kxxHash`, the RocksDB's version upgraded from 6.10.2 to 7.9.2, and rolling back to 6.10.2 works fine. ### More to discuss When writing the unit test to read the table checksum type from RocksDB configuration files, it failed. I found the related issue on RocksDB: facebook/rocksdb#5297 The related PR: facebook/rocksdb#10826 It means we still can't load RocksDB table options from configuration files. Maybe I missed some parts about reading RocksDB table options from the configuration file. If this issue exists, we do **NOT** recommend users configure RocksDB configurations through configuration files. @merlimat @eolivelli @dlg99 Please help take a look, thanks.
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
Fix #3734 (comment)
We have two rocksDB tables, one for the ledger index, and another for the entry log location.
kCRC32ckxxHashWhen we upgrade the RocksDB version from 6.10.2 to 7.9.2, the new RocksDB version's default table checksum has changed from
kCRC32ctokXXH3, andkXXH3only supported since RocksDB 6.27. The RocksDB version rollback to 6.10.2 will be failed due to RocksDB 6.10.2 doesn't support thekXXH3checksum type.Modifications
In this PR, I make the RocksDB checksum type configurable. But there is one change that will change the ledger index RocksDB table's checksum type from the default
kCRC32ctokxxHash. I have tested the compatibility of the two checksum types in and between multiple RocksDB versions, it works fine.After setting the two RocksDB table's checksum type to
kxxHash, the RocksDB's version upgraded from 6.10.2 to 7.9.2, and rolling back to 6.10.2 works fine.More to discuss
When writing the unit test to read the table checksum type from RocksDB configuration files, it failed. I found the related issue on RocksDB: facebook/rocksdb#5297
The related PR: facebook/rocksdb#10826
It means we still can't load RocksDB table options from configuration files. Maybe I missed some parts about reading RocksDB table options from the configuration file.
If this issue exists, we do NOT recommend users configure RocksDB configurations through configuration files.
@merlimat @eolivelli @dlg99 Please help take a look, thanks.