[cleanup][broker] Fix PersistentSubscription duplicate implementation interface Subscription

[sherlock-lin]

…ation interface Subscription

Fixes #22354

Main Issue: #xyz

PIP: #xyz

Motivation

Modifications

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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:

[dao-jun]

lgtm

[liangyuanpeng]

/pulsarbot rerun-failure-checks

[sherlock-lin]

/pulsarbot rerun-failure-checks

What should I do please, I haven't changed the file that failed detection

[liangyuanpeng]

Seems like it is:

• [fix] [test] Fix flaky test ManagedLedgerTest.testGetNumberOfEntriesInStorage #22344

It's merged now.

[lhotari]

This isn't a real problem at all. In the Pulsar code base, we currently try to minimize unnecessary changes. This might change after we move to a different type of maintenance strategy, the dev mailing list discussion is https://lists.apache.org/thread/j6qvt45rndnvjypcmqxsfmddqt41bxjv . Refactorings aren't currently mentioned there explicitly.

[lhotari]

Just to say that it's better to not create a lot of similar PRs since that would be a lot of changes which don't provide real value. We have a lot of technical debt with more important, but low value areas such as compiler warnings. There's a lot of compiler warnings when compiling Pulsar. We have chosen to ignore them for now. I hope this could change after we have a better maintenance strategy in place.

[Technoboy-]

This isn't a real problem at all. In the Pulsar code base, we currently try to minimize unnecessary changes. This might change after we move to a different type of maintenance strategy, the dev mailing list discussion is https://lists.apache.org/thread/j6qvt45rndnvjypcmqxsfmddqt41bxjv . Refactorings aren't currently mentioned there explicitly.

I don't agree with you for this patch. This isn't a problem, but from the Java interface definition, it's better to do like this.

[Technoboy-]

Just to say that it's better to not create a lot of similar PRs since that would be a lot of changes which don't provide real value. We have a lot of technical debt with more important, but low value areas such as compiler warnings. There's a lot of compiler warnings when compiling Pulsar. We have chosen to ignore them for now. I hope this could change after we have a better maintenance strategy in place.

we can close other patch that not for master branch

[lhotari]

I don't agree with you for this patch. This isn't a problem, but from the Java interface definition, it's better to do like this.

@Technoboy- I agree that it is "better". When something is "better", this implies that it produces some benefit. In this case, what is it that it makes better? The question is then whether this benefit is relevant when we are considering how Pulsar is currently maintained. I've provided my opinion about formatting changes in this mailing list post: https://lists.apache.org/thread/lo15cdzsl740dwgcqwpsl9oy9qb13onv . After we settle on a maintenance strategy that reduces merge conflicts for maintaining the LTS branch, we have more freedom to do refactorings which we are currently avoiding because of the merge conflicts. Merge conflicts caused by unnecessary changes are the main reason for my resistance. That could be resolved with an improved maintenance strategy.

[sherlock-lin]

This is my first pr, if the merge in can give me more confidence, I'll try to avoid mentioning low value pr taking up resources until the technical debt clears up.

[codelipenghui]

+1 for merging this PR, but it's better to change the title to "[cleanup][broker] xxx".

And I don't think it will introduce more issues for cherry-picking. Cleanup is also welcome.
Let's run case by case.

[Technoboy-]

This is my first pr, if the merge in can give me more confidence, I'll try to avoid mentioning low value pr taking up resources until the technical debt clears up.

Yes, we know. Thanks

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.63%. Comparing base (bbc6224) to head (fcde506).
Report is 99 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #22359      +/-   ##
============================================
+ Coverage     73.57%   73.63%   +0.06%     
+ Complexity    32624    32125     -499     
============================================
  Files          1877     1879       +2     
  Lines        139502   140277     +775     
  Branches      15299    15571     +272     
============================================
+ Hits         102638   103299     +661     
- Misses        28908    28967      +59     
- Partials       7956     8011      +55     

Flag	Coverage Δ
inttests	27.28% <ø> (+2.69%)	⬆️
systests	24.62% <ø> (+0.30%)	⬆️
unittests	72.88% <ø> (+0.03%)	⬆️

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

Files	Coverage Δ
...rvice/nonpersistent/NonPersistentSubscription.java	53.27% <ø> (ø)
...ker/service/persistent/PersistentSubscription.java	76.72% <ø> (+0.03%)	⬆️

... and 150 files with indirect coverage changes

[lhotari]

This is my first pr, if the merge in can give me more confidence, I'll try to avoid mentioning low value pr taking up resources until the technical debt clears up.

@sherlock-lin Congrats for the first PR and welcome to join the Pulsar contributors! For the purpose of getting familiar to how PRs are merged and how the Pulsar CI works, this PR was a great way to learn that.

You might notice in future PRs that we have quite a few flaky tests in Pulsar CI and usually there will be multiple flakes and the build will have to be retried multiple times. For this PR, you hadn't yet tried out the "Personal CI" option for running PR builds in your own fork to get build feedback. That's useful to learn since you can retry failing builds as you like in your own fork and prepare the PR for final review. The guide is missing some details since you will have to enable GitHub Actions in your Pulsar fork. Contributions to the instructions are also welcome if you notice any gaps.

I'm sorry about the criticism about this PR. I got stuck on the detail that this PR isn't a bug fix, but a code cleanup PR and ended up explaining more details why I'm not eager to encourage to do more code cleanup PRs before we have changed the Pulsar maintenance strategy (#22359 (comment)).

We have plenty of valuable contribution opportunies in the Apache Pulsar project. Please join the Apache Pulsar Slack and it's #dev channel. When you are looking for contribution opportunies, I'll be happy to suggest valuable opportunies for you and help with any barriers for contributions.

/cc @Technoboy- @codelipenghui

[sherlock-lin]

This is my first pr, if the merge in can give me more confidence, I'll try to avoid mentioning low value pr taking up resources until the technical debt clears up.

@sherlock-lin Congrats for the first PR and welcome to join the Pulsar contributors! For the purpose of getting familiar to how PRs are merged and how the Pulsar CI works, this PR was a great way to learn that.

You might notice in future PRs that we have quite a few flaky tests in Pulsar CI and usually there will be multiple flakes and the build will have to be retried multiple times. For this PR, you hadn't yet tried out the "Personal CI" option for running PR builds in your own fork to get build feedback. That's useful to learn since you can retry failing builds as you like in your own fork and prepare the PR for final review. The guide is missing some details since you will have to enable GitHub Actions in your Pulsar fork. Contributions to the instructions are also welcome if you notice any gaps.

I'm sorry about the criticism about this PR. I got stuck on the detail that this PR isn't a bug fix, but a code cleanup PR and ended up explaining more details why I'm not eager to encourage to do more code cleanup PRs before we have changed the Pulsar maintenance strategy (#22359 (comment)).

We have plenty of valuable contribution opportunies in the Apache Pulsar project. Please join the Apache Pulsar Slack and it's #dev channel. When you are looking for contribution opportunies, I'll be happy to suggest valuable opportunies for you and help with any barriers for contributions.

/cc @Technoboy- @codelipenghui

Thanks @lhotari for your patient guidance! in this PR submission I have irregularities thanks for pointing out.

in this process I have learned a lot about contributing code knowledge, I will learn more about submitting pr specification knowledge to improve efficiency!

Also thanks to @Technoboy- @codecov-commenter @liangyuanpeng and other partners for their guidance and help. I'm honored to be able to join Pulsar contributors with everybody to make Pulsar better!