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 2021/01/18 16:22:35 UTC

[GitHub] [pulsar] eolivelli opened a new pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

eolivelli opened a new pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232


   ### Motivation
   
   BookKeeper 4.12 introduces the 'Opportunistic Striping' feature.
   In BK terms 'striping' happens when EnsembleSize is greater than WriteQuorumSize, in this mode the entries are distributed round robin over a set of bookies, in order to  achieve better performances as you can use the resources of more bookies.
   
   For instance in a small HA cluster, with only 3 bookies, you must run Pulsar with 2-2-2 replication parameters (EnsembleSize=2,WriteQuorumSize=2,AckQuorumSize=2).
   You cannot set EnsembleSize=3 (and thus use 'striping') because in case of temporary outage of a single bookie the BK client is not able to create an ensemble with 3 bookies.
   
   With Opportunistic Striping you can use 3-2-2 and when the system is fully up-and-running with 3 bookies then you go with striping, but during single bookie outages (like during reconfigurations/updates) you fall back to 2-2-2. 
   
   This is not about consistency or durability, it is only about having the ability to get the most out of your bookkeeper cluster.
   
   ### Modifications
   
   Add new configuration option
   
   ### Verifying this change
   
   - the change add new tests
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If `yes` was chosen, please highlight the changes*
   
     - Dependencies: no
     - The public API: no
     - The schema: no
     - The default values of configurations:  no
     - The wire protocol: no
     - The rest endpoints: no
     - The admin cli options: no
     - Anything that affects deployment:  no
     
     - ### Documentation
   
     - Does this pull request introduce a new feature? yes
     - If yes, how is the feature documented? JavaDocs and default configuration files
     


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-766622808


   @sijie can you please take another look ?


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

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

[GitHub] [pulsar] zymap commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-829783249


   @eolivelli Actually this feature is added from bookkeeper 4.13.0. We are still using 4.12.0 in 2.7.x. This PR can not cherry-pick to branch-2.7. Could you please revert these changes?


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

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

[GitHub] [pulsar] eolivelli commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r559802595



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,10 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+bookkeeperOpportunisticStriping=false

Review comment:
       I am open to switch to a new mode of configuring the internal BK client.
   
   If I understand correctly currently in Pulsar community we prefer to allow only a limited and known set of BK configuration options in order to have total control over which feature of BK can be safely used and that we are officially supporting in Pulsar 
   
   If we let any configuration option to be activated users could fall into bad conditions that are not supposed to happen in Pulsar.
   
   Can you confirm that a more generic approach is what we want?




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

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

[GitHub] [pulsar] sijie merged pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
sijie merged pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232


   


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-766622808


   @sijie can you please take another look ?


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

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

[GitHub] [pulsar] lhotari commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r560173651



##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1070,6 +1070,14 @@
             + " and resolving its metadata service location"
     )
     private String bookkeeperMetadataServiceUri;
+    @FieldContext(
+        category = CATEGORY_STORAGE_BK,
+        doc = "With Opportustic Striping the bookeeper client will fallback to use less bookies in case "

Review comment:
       A few typos, fixed:
   "With Opportunistic Striping the bookkeeper client will fall back to use fewer bookies in 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.

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-763498790


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-765204901


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] sijie commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r561821943



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,13 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+#bookkeeper.opportunisticStriping=false

Review comment:
       Can we avoid using "."? Currently, we use system environment variables for loading and applying config in Kubernetes. Using "." will make things become challenging. Can we use `bookkeeper_` instead?




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

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

[GitHub] [pulsar] eolivelli edited a comment on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-763589169


   @sijie  I have updated the patch according to your suggestion PTAL


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-762742335


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-767675139


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] eolivelli commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r561893885



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,13 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+#bookkeeper.opportunisticStriping=false

Review comment:
       yes, good idea @sijie .
   I have updated the patch
   




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

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

[GitHub] [pulsar] sijie commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r560090307



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,10 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+bookkeeperOpportunisticStriping=false

Review comment:
       I think we should provide a generic approach instead of patching individual configurations.




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

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

[GitHub] [pulsar] eolivelli commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r561893885



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,13 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+#bookkeeper.opportunisticStriping=false

Review comment:
       yes, good idea @sijie .
   I have updated the patch
   




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

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

[GitHub] [pulsar] eolivelli commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r560177837



##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1070,6 +1070,14 @@
             + " and resolving its metadata service location"
     )
     private String bookkeeperMetadataServiceUri;
+    @FieldContext(
+        category = CATEGORY_STORAGE_BK,
+        doc = "With Opportustic Striping the bookeeper client will fallback to use less bookies in case "

Review comment:
       thank you @lhotari 
   I have fixed this line according to your suggestion




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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-769912393


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-763589169


   @sijie  I have updated the patch according to your suggestion
   
   @lhotari please take another look as the patch changed


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

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

[GitHub] [pulsar] sijie commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r559774996



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,10 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+bookkeeperOpportunisticStriping=false

Review comment:
       I think we should provide a more generic method to load and customize bookkeeper settings instead of adding them one by one.




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

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

[GitHub] [pulsar] sijie commented on a change in pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#discussion_r561821943



##########
File path: conf/broker.conf
##########
@@ -800,6 +800,13 @@ managedLedgerDefaultWriteQuorum=2
 # Number of guaranteed copies (acks to wait before write is complete)
 managedLedgerDefaultAckQuorum=2
 
+# with OpportunisticStriping=true the ensembleSize is adapted automatically to writeQuorum
+# in case of lack of enough bookies
+#bookkeeper.opportunisticStriping=false

Review comment:
       Can we avoid using "."? Currently, we use system environment variables for loading and applying config in Kubernetes. Using "." will make things become challenging. Can we use `bookkeeper_` instead?




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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-762715291


   /pulsarbot run-failure-checks


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

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

[GitHub] [pulsar] eolivelli commented on pull request #9232: Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix)

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9232:
URL: https://github.com/apache/pulsar/pull/9232#issuecomment-829805824


   Let me fix it ASAP.
   Sorry for the noise.
   I wanted to pick this patch in order to let users configure internal BK client, not OpportunisticStriping feature.
   
   I will send a PR that cleans up the branch with a partial revert 


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

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