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

[cleanup][broker] PIP 45: Deprecate zookeeper settings managedLedgerMaxUnackedRangesToPersistInZooKeeper #15099

Merged
merged 8 commits into from
Apr 15, 2022

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Apr 10, 2022

Motivation

To deprecate the zookeeper settings. PIP 45 introduced a unified metadata store API for Pulsar,
so we should make the metadata-related configuration start with metadataStore, not zooKeeper/ZK.

Modifications

  • Change managedLedgerMaxUnackedRangesToPersistInZooKeeper to managedLedgerMaxUnackedRangesToPersistInMetadataStore
  • Change ML config maxUnackedRangesToPersistInZk to maxUnackedRangesToPersistInMetadataStore.

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

  • The default values of configurations: (yes)

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

Compatibility

Introduce getManagedLedgerMaxUnackedRangesToPersistInMetadataStore method to compatible with previous configurations.

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Apr 10, 2022
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

Need to update broker.conf, too.

@mattisonchao
Copy link
Member Author

Need to update broker.conf, too.

Fixed, thanks to you.

@Anonymitaet
Copy link
Member

@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@momo-jun
Copy link
Contributor

@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks

In case this PR (for ZK free) is merged into 2.10, I drafted a related doc PR to catch up.

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Apr 12, 2022
conf/broker.conf Outdated
# than this limit then broker will persist unacked ranges into bookkeeper to avoid additional data overhead into
# zookeeper.
# Deprecated: use managedLedgerMaxUnackedRangesToPersistInMetadataStore
managedLedgerMaxUnackedRangesToPersistInZooKeeper=1000
Copy link
Contributor

Choose a reason for hiding this comment

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

You can change the default value of the deprecated to -1
and then check if users changed -1 to other values, we should apply this one.

A related PR https://github.com/apache/pulsar/pull/14147/files#diff-cc761e782083f37db72cd91684fee07b931c188dd93333397c62b0a4c45a657eR2671-R2681

@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 12, 2022
Copy link
Member

@shibd shibd left a comment

Choose a reason for hiding this comment

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

LGTM

@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codelipenghui codelipenghui merged commit 8c534db into apache:master Apr 15, 2022
@codelipenghui codelipenghui changed the title [cleanup][broker] PIP 45: Deprecate zookeeper settings [cleanup][broker] PIP 45: Deprecate zookeeper settings managedLedgerMaxUnackedRangesToPersistInZooKeeper Apr 15, 2022
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Apr 18, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-complete Your PR changes impact docs and the related docs have been already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants