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.

Sign up for GitHub

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

Jump to bottom

[improve][pip] PIP-376: Make topic policies service pluggable #23248

Merged
merged 9 commits into from
Sep 11, 2024
Merged

[improve][pip] PIP-376: Make topic policies service pluggable #23248

merged 9 commits into from
Sep 11, 2024

Conversation

BewareMyPower
Copy link
Contributor

Documentation

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

@github-actions github-actions bot added the PIP label Sep 3, 2024
@BewareMyPower BewareMyPower self-assigned this Sep 3, 2024
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 3, 2024
@BewareMyPower
Copy link
Contributor Author

Here is the draft implementation in my fork: BewareMyPower#35

eolivelli
eolivelli reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I generally support this proposal, we should have clearer APIs for the components we want to allow users to customize

pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
@dao-jun
Copy link
Member

dao-jun commented Sep 3, 2024

I support

codelipenghui
codelipenghui reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

@BewareMyPower Thanks for driving the proposal
Could you please also add a section to describe what the TopicPoliciesService finally looks like?

pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
pip/pip-376.md Outdated Show resolved Hide resolved
lhotari
lhotari reviewed Sep 4, 2024
View reviewed changes
pip/pip-376.md Outdated Show resolved Hide resolved
@BewareMyPower BewareMyPower marked this pull request as draft September 4, 2024 08:26
@BewareMyPower BewareMyPower marked this pull request as ready for review September 4, 2024 10:43
@BewareMyPower
Copy link
Contributor Author

Now I've refactored the proposal, as well as the demo implementation with the following changes:

  1. Do not mention other plugins. Just mention the default implementation might not always be best.
  2. Change LOCAL_FIRST to DEFAULT.
  3. List the whole interface after this change.
  4. Refactored the document with the help of GitHub Copilot.

@eolivelli @codelipenghui @lhotari PTAL again

lhotari
lhotari approved these changes Sep 4, 2024
View reviewed changes
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. Good work @BewareMyPower!

RobertIndie
RobertIndie reviewed Sep 6, 2024
View reviewed changes
pip/pip-376.md Outdated Show resolved Hide resolved
RobertIndie
RobertIndie approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

LGTM. Just one minor comment: #23248 (comment)

@BewareMyPower BewareMyPower merged commit a8ae3e4 into apache:master Sep 11, 2024
20 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/pip-376-pluggable-topic-policies-doc branch September 11, 2024 03:20
michalcukierman pushed a commit to michalcukierman/pulsar that referenced this pull request Sep 11, 2024
@BewareMyPower @michalcukierman
[improve][pip] PIP-376: Make topic policies service pluggable (apache…
4592a08 
…#23248)
@BewareMyPower BewareMyPower mentioned this pull request Sep 19, 2024
Merged
4 tasks
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli left review comments

@RobertIndie RobertIndie RobertIndie approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@lhotari lhotari lhotari approved these changes

@shibd shibd Awaiting requested review from shibd

Assignees

@BewareMyPower BewareMyPower

Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. PIP
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@BewareMyPower @dao-jun @lhotari @eolivelli @codelipenghui @RobertIndie