[improve][broker] Add config fsyncEnable for RocksdbMetadataStore - #18801
Conversation
github-actions
Bot
added
the
doc-required
5bf5bc9 to
979c37a
Compare
coderzc
added
doc-not-needed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18801 +/- ##
============================================
- Coverage 50.05% 47.73% -2.33%
+ Complexity 11024 10651 -373
============================================
Files 703 703
Lines 68814 68831 +17
Branches 7378 7378
============================================
- Hits 34446 32855 -1591
- Misses 30621 32319 +1698
+ Partials 3747 3657 -90
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| # Configuration file path for metadata store. It's supported by RocksdbMetadataStore and EtcdMetadataStore for now | ||
| metadataStoreConfigPath= | ||
|
|
||
| # Whether we should enable fsync for local metadata store. It's supported by RocksdbMetadataStore for now |
There was a problem hiding this comment.
It would be more clear to add this: "If this flag is true, metadata writes will be slower. If this flag is false, and the machine crashes, some recent metadata writes may be lost. Note that if it is just the process that crashes (i.e., the machine does not reboot), no writes will be lost even if it is false."
| public void testPut(String provider, Supplier<String> urlSupplier) throws Exception { | ||
| @Cleanup | ||
| MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); | ||
| MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), |
There was a problem hiding this comment.
NIT: This benchmark is not even a unit test. It's more like a perf tool.
| MetadataStoreExtended store = MetadataStoreExtended.create(urlSupplier.get(), | ||
| MetadataStoreConfig.builder().fsyncEnable(false).build()); |
There was a problem hiding this comment.
Please also help check other tests that use the fsync mode.
| # If this flag is true, metadata writes will be slower. | ||
| # If this flag is false, and the machine crashes, some recent metadata writes may be lost. | ||
| # Note that if it is just the process that crashes (i.e., the machine does not reboot), no writes will be lost even if it is false. | ||
| metadataFsyncEnabled=true |
There was a problem hiding this comment.
Maybe we don't need to expose it to users?
We just want to improve the test.
Suppose we allow users to configure with false. The machine crash might lead to the metadata being corrupted.
There was a problem hiding this comment.
Or we can expose it until we need it. I haven't seen any issues or discussions about this part.
And batch operations might be another choice to improve the rocksdb metadata performance.
There was a problem hiding this comment.
Maybe we don't need to expose it to users? We just want to improve the test. Suppose we allow users to configure with false. The machine crash might lead to the metadata being corrupted.
@codelipenghui Rocksdb still can sync wal/memtable file by wal_bytes_per_sync and bytes_per_sync config, I think when setting wal_bytes_per_sync=1 metadata no write will be lost even if it is false.
Whether we should automatically set wal_bytes_per_sync=1 and set WriteOptions.sync=false ?
There was a problem hiding this comment.
Whether we should automatically set wal_bytes_per_sync=1 and set WriteOptions.sync=false ?
IIUC, WriteOptions.sync only applies fsync on WAL. So there is no need to config wal_bytes_per_sync.
There was a problem hiding this comment.
Or we can expose it until we need it.
+1, we should be more cautious about exposing more configs to users. I think we can keep the config of MetadataStoreConfig and remove the one in ServiceConfiguration. It would be enough to solve the unit test case.
There was a problem hiding this comment.
Whether we should automatically set wal_bytes_per_sync=1 and set WriteOptions.sync=false ?
IIUC,
WriteOptions.synconly appliesfsyncon WAL. So there is no need to configwal_bytes_per_sync.
In my local test, if enable WriteOptions.sync, PulsarLedgerIdGeneratorTest.testGenerateLedgerId will timeout but only set wal_bytes_per_sync=1 it does not timeout
There was a problem hiding this comment.
Or we can expose it until we need it.
+1, we should be more cautious about exposing more configs to users. I think we can keep the config of
MetadataStoreConfigand remove the one inServiceConfiguration. It would be enough to solve the unit test case.
OK.
coderzc
changed the title
metadataFsyncEnabled for RocksdbMetadataStorefsyncEnable for RocksdbMetadataStore
4 participants
Motivation
Now, RocksdbMetadataStore enables
WriteOptions.sync, which will make writing slower, I add a configmetadataFsyncEnabledto allow user to disablefsyncaccording to the need.Modifications
Add config
fsyncEnableforRocksdbMetadataStore.Verifying this change
Such as
PulsarLedgerIdGeneratorTesttest execution time lessDocumentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: