Skip to content

[fix][broker] Support zookeeper read-only config. - #19156

Merged
Technoboy- merged 3 commits into
apache:masterfrom
horizonzy:support-read-only-config
Jan 14, 2023
Merged

[fix][broker] Support zookeeper read-only config.#19156
Technoboy- merged 3 commits into
apache:masterfrom
horizonzy:support-read-only-config

Conversation

@horizonzy

@horizonzy horizonzy commented Jan 8, 2023

Copy link
Copy Markdown
Member

Fixes #16489

Master Issue: #16489

Motivation

Export config metadataStoreAllowReadOnlyOperations and zooKeeperAllowReadOnlyOperations.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 8, 2023
)
private int zooKeeperCacheExpirySeconds = -1;

@Deprecated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we add deprecated fields? Seems not necessary.

@horizonzy horizonzy Jan 8, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If the user uses the old properties, it config zookeeperAllowReadOnlyOperations=true enough, instead to comprehend the new config.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we remove the zooKeeperAllowReadOnlyOperations config? Because there seems is no compatible issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems we need to remove it.

@codecov-commenter

codecov-commenter commented Jan 9, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.61%. Comparing base (da87e40) to head (55d4543).
⚠️ Report is 3176 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/apache/pulsar/proxy/server/ProxyService.java 0.00% 4 Missing ⚠️
...n/java/org/apache/pulsar/broker/PulsarService.java 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19156      +/-   ##
============================================
+ Coverage     46.42%   47.61%   +1.18%     
- Complexity    10437    10824     +387     
============================================
  Files           703      713      +10     
  Lines         68816    69736     +920     
  Branches       7377     7497     +120     
============================================
+ Hits          31950    33203    +1253     
+ Misses        33252    32823     -429     
- Partials       3614     3710      +96     
Flag Coverage Δ
unittests 47.61% <28.57%> (+1.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...apache/pulsar/proxy/server/ProxyConfiguration.java 97.46% <100.00%> (+0.01%) ⬆️
...n/java/org/apache/pulsar/broker/PulsarService.java 56.57% <50.00%> (-0.23%) ⬇️
...a/org/apache/pulsar/proxy/server/ProxyService.java 80.18% <0.00%> (-0.75%) ⬇️

... and 133 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@horizonzy

Copy link
Copy Markdown
Member Author

@Technoboy- 2.9.x 2.10.x 2.11.x also need this pr.

@mattisonchao mattisonchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM,

But I have to mention one thing If you want to cherry-pick this PR to branch-2.9, you've got to support and cherry-pick zooKeeperAllowReadOnlyOperations configuration first and then push another PR to introduce metadataStoreAllowReadOnlyOperations and deprecate the previous one.

The reason is that branch-2.9 has no metadata**** configuration. So, if we introduce a new config named metadata****, it's weird.

Please let me know if you have any concerns. Thanks a lot!
/cc @Technoboy- @Jason918

@horizonzy

horizonzy commented Jan 12, 2023

Copy link
Copy Markdown
Member Author

LGTM,

But I have to mention one thing If you want to cherry-pick this PR to branch-2.9, you've got to support and cherry-pick zooKeeperAllowReadOnlyOperations configuration first and then push another PR to introduce metadataStoreAllowReadOnlyOperations and deprecate the previous one.

The reason is that branch-2.9 has no metadata**** configuration. So, if we introduce a new config named metadata****, it's weird.

I appreciate your reminder.

@mattisonchao mattisonchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The author understands the problem I mentioned and the code LGTM, I will approve it and the author will choose a good approach to solve the problem.

@horizonzy

horizonzy commented Jan 13, 2023

Copy link
Copy Markdown
Member Author

This is part 1, introduce zooKeeperAllowReadOnlyOperations. It should cherry-pick to branch-2.9, branch-2.10, branch-2.11.
After the cherry-pick works done, I will submit a new pr to deprecated zooKeeperAllowReadOnlyOperations and introduce metadataStoreAllowReadOnlyOperations. It will cherry-pick to branch-2.10, branch-2.11

@Jason918 Jason918 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@hangc0276

Copy link
Copy Markdown
Contributor

@horizonzy Do we need to set the default value for zooKeeperAllowReadOnlyOperations?

@horizonzy

horizonzy commented Feb 27, 2023

Copy link
Copy Markdown
Member Author

zooKeeperAllowReadOnlyOperations

Yes, it need. I will add it in the broker.conf.

liangyepianzhou added a commit that referenced this pull request Mar 1, 2023
coderzc pushed a commit that referenced this pull request Mar 3, 2023
@coderzc coderzc added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Mar 3, 2023
Annavar-satish pushed a commit to pandio-com/pulsar that referenced this pull request Mar 6, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 9, 2023
…) (apache#19637)

Co-authored-by: Yan Zhao <horizonzy@apache.org>
(cherry picked from commit a8f3c81)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pulsar no longer works when zookeeper global goes into read only