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] Remove unused cache executor in PulsarService #20563

Merged
merged 1 commit into from Jun 20, 2023

Conversation

lifepuzzlefun
Copy link
Contributor

@lifepuzzlefun lifepuzzlefun commented Jun 12, 2023

Motivation

Remove unused cache executor in PulsarService

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

Matching PR in forked repository

PR in forked repository:

@github-actions
Copy link

@lifepuzzlefun Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jun 12, 2023
@AnonHxy
Copy link
Contributor

AnonHxy commented Jun 14, 2023

LGTM

@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 17, 2023
@Technoboy- Technoboy- added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/broker labels Jun 17, 2023
@AnonHxy AnonHxy merged commit 677d160 into apache:master Jun 20, 2023
45 of 48 checks passed
Comment on lines -1457 to -1459
public ScheduledExecutorService getCacheExecutor() {
return cacheExecutor;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a breaking change for pulsar extensions. I don't know of any extensions using it, but it seems risky to just remove the method. @BewareMyPower - what is your opinion on the right way to handle these kinds of methods that could be used by extensions?

Copy link
Contributor

Choose a reason for hiding this comment

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

A better way might be adding the @Deprecated annotation to all these public methods first, then the extension developer could be aware that these methods could be removed in future.

But I think it's reasonable to remove unused methods in Pulsar, the extension should not depend on public methods from a class (rather than an interface) too much. Public methods are added too casually. Just like #20537, authors might just add a getter (e.g. NamespaceBundles#getNsname) for a very limited use case and they might not realize they have added interfaces that could bring breaking changes if they are removed.

If we want to guarantee all public methods in classes that should not be removed directly, we should guarantee any public method should not be added without any care. Otherwise, there could be too many public methods that require PIPs to remove them.

However, if so, it could be a burden to Pulsar developers that each new public method requires a PIP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants