You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/07 13:00:38 UTC

[GitHub] [pulsar] coderzc opened a new pull request, #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

coderzc opened a new pull request, #18801:
URL: https://github.com/apache/pulsar/pull/18801

   ### Motivation
   
   Now, RocksdbMetadataStore enables `WriteOptions.sync`, which will make writing slower, I add a config `metadataFsyncEnabled` to allow user to disable `fsync` according to the need.
   
   ### Modifications
   
   Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`.
   
   ### Verifying this change
   
   The `PulsarLedgerIdGeneratorTest` and `MetadataBenchmark` execution time less
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [x] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui merged pull request #18801: [improve][broker] Add config `fsyncEnable` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #18801:
URL: https://github.com/apache/pulsar/pull/18801


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.
   
   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. 
   
   https://github.com/facebook/rocksdb/blob/534fb06dd38234a2d6234d17366a7b0ac5272f48/include/rocksdb/options.h#L981-L1004



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044171906


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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`.
   
   In my local test, if enable `WriteOptions.sync`, `PulsarLedgerIdGeneratorTest.testGenerateLedgerId` will timeout but only set `wal_bytes_per_sync=1 ` it does not timeout



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.
   
   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. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Jason918 commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1042884940


##########
conf/standalone.conf:
##########
@@ -29,6 +29,8 @@ metadataStoreUrl=
 # 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

Review 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."



##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataBenchmark.java:
##########
@@ -108,7 +108,8 @@ public void testGetChildren(String provider, Supplier<String> urlSupplier) throw
     @Test(dataProvider = "impl", enabled = 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(),

Review Comment:
   NIT: This benchmark is not even a unit test. It's more like a perf tool.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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. 
   
   https://github.com/facebook/rocksdb/blob/534fb06dd38234a2d6234d17366a7b0ac5272f48/include/rocksdb/options.h#L981-L1004
   
   Whether we should automatically set `wal_bytes_per_sync=1` and set `WriteOptions.sync=false`   ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1043951351


##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java:
##########
@@ -49,8 +49,8 @@ public class PulsarLedgerIdGeneratorTest extends BaseMetadataStoreTest {
     @Test(dataProvider = "impl")
     public void testGenerateLedgerId(String provider, Supplier<String> urlSupplier) throws Exception {
         @Cleanup
-        MetadataStoreExtended store =
-                MetadataStoreExtended.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
+        MetadataStoreExtended store = MetadataStoreExtended.create(urlSupplier.get(),
+                        MetadataStoreConfig.builder().fsyncEnable(false).build());

Review Comment:
   Please also help check other tests that use the fsync mode.



##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.



##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#issuecomment-1340996135

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/18801?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18801](https://codecov.io/gh/apache/pulsar/pull/18801?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (979c37a) into [master](https://codecov.io/gh/apache/pulsar/commit/68ca60cea7665e6a4bd9f07518b038c17893fe02?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (68ca60c) will **decrease** coverage by `13.68%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/18801/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/18801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #18801       +/-   ##
   =============================================
   - Coverage     50.05%   36.37%   -13.69%     
   + Complexity    11024     6502     -4522     
   =============================================
     Files           703      597      -106     
     Lines         68814    56821    -11993     
     Branches       7378     5899     -1479     
   =============================================
   - Hits          34446    20669    -13777     
   - Misses        30621    33586     +2965     
   + Partials       3747     2566     -1181     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `36.37% <0.00%> (-13.69%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...n/java/org/apache/pulsar/broker/PulsarService.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9QdWxzYXJTZXJ2aWNlLmphdmE=) | `60.87% <0.00%> (-3.53%)` | :arrow_down: |
   | [...ava/org/apache/pulsar/broker/admin/v1/Brokers.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9Ccm9rZXJzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...va/org/apache/pulsar/broker/admin/v1/Clusters.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9DbHVzdGVycy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../org/apache/pulsar/broker/admin/v1/Properties.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9Qcm9wZXJ0aWVzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ar/common/naming/PartitionedManagedLedgerInfo.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvUGFydGl0aW9uZWRNYW5hZ2VkTGVkZ2VySW5mby5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...naming/RangeEquallyDivideBundleSplitAlgorithm.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvUmFuZ2VFcXVhbGx5RGl2aWRlQnVuZGxlU3BsaXRBbGdvcml0aG0uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...e/pulsar/broker/admin/impl/ResourceQuotasBase.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1Jlc291cmNlUXVvdGFzQmFzZS5qYXZh) | `0.00% <0.00%> (-96.43%)` | :arrow_down: |
   | [.../pulsar/broker/rest/RestMessagePublishContext.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9yZXN0L1Jlc3RNZXNzYWdlUHVibGlzaENvbnRleHQuamF2YQ==) | `0.00% <0.00%> (-92.60%)` | :arrow_down: |
   | [...he/pulsar/broker/service/AnalyzeBacklogResult.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0FuYWx5emVCYWNrbG9nUmVzdWx0LmphdmE=) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
   | [.../apache/pulsar/broker/admin/v1/ResourceQuotas.java](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9SZXNvdXJjZVF1b3Rhcy5qYXZh) | `0.00% <0.00%> (-91.43%)` | :arrow_down: |
   | ... and [240 more](https://codecov.io/gh/apache/pulsar/pull/18801/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Jason918 commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044137691


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.
   
   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. 
   
   ref: https://github.com/facebook/rocksdb/wiki/WAL-Performance



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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. 
   
   https://github.com/facebook/rocksdb/blob/534fb06dd38234a2d6234d17366a7b0ac5272f48/include/rocksdb/options.h#L981-L1004



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `fsyncEnable` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044200247


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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.
   
   OK.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044046819


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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. 
   
   https://github.com/facebook/rocksdb/blob/534fb06dd38234a2d6234d17366a7b0ac5272f48/include/rocksdb/options.h#L981-L1004
   
   Whether we should automatically set `wal_bytes_per_sync=1`  ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Jason918 commented on a diff in pull request #18801: [improve][broker] Add config `metadataFsyncEnabled` for `RocksdbMetadataStore`

Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #18801:
URL: https://github.com/apache/pulsar/pull/18801#discussion_r1044135877


##########
conf/standalone.conf:
##########
@@ -29,6 +29,11 @@ metadataStoreUrl=
 # 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
+# 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

Review 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`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org