Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix) #9232

Merged
merged 9 commits into from Feb 1, 2021

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Jan 18, 2021

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 a generic way to configure internal BookKeeper client options, any entry start starts with bookkeeper_ is passed to the BK client configuration after stripping the prefix
  • Add unit tests for bookkeeper_opportunisticStriping configuration option
  • Add configuration example in broker.conf

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

conf/broker.conf Outdated Show resolved Hide resolved
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sijie sijie added this to the 2.8.0 milestone Jan 20, 2021
@sijie sijie added the type/feature The PR added a new feature or issue requested a new feature label Jan 20, 2021
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

eolivelli commented Jan 20, 2021

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

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

conf/broker.conf Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eolivelli eolivelli changed the title Allow to configure BookKeeper Opportunistic Striping Feature Allow to configure BookKeeper Opportunistic Striping Feature (and all BK client features using bookkeeper_ prefix) Jan 21, 2021
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

@sijie can you please take another look ?

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie merged commit bc69ad2 into apache:master Feb 1, 2021
eolivelli added a commit that referenced this pull request Apr 29, 2021
… BK client features using bookkeeper_ prefix) (#9232)

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.

- Add a generic way to configure internal BookKeeper client options, any entry start starts with `bookkeeper_` is passed to the BK client configuration after stripping the prefix
- Add unit tests for `bookkeeper_opportunisticStriping` configuration option
- Add configuration example in broker.conf

- the change add new tests

(cherry picked from commit bc69ad2)
@eolivelli eolivelli added cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.2 labels Apr 29, 2021
@zymap
Copy link
Member

zymap commented Apr 30, 2021

@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?

zymap added a commit to streamnative/pulsar-archived that referenced this pull request Apr 30, 2021
…(and all BK client features using bookkeeper_ prefix) (apache#9232)"

This reverts commit baddf9d.
@eolivelli
Copy link
Contributor Author

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

zymap added a commit to streamnative/pulsar-archived that referenced this pull request Apr 30, 2021
…(and all BK client features using bookkeeper_ prefix) (apache#9232)"

This reverts commit baddf9d.
timmyyuan pushed a commit to streamnative/pulsar-archived that referenced this pull request May 8, 2021
…(and all BK client features using bookkeeper_ prefix) (apache#9232)"

This reverts commit baddf9d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.2 type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants