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

[fix][client] Move MessageIdAdv to the pulsar-common module #20139

Merged

Conversation

BewareMyPower
Copy link
Contributor

Motivation

#19414 does not follow the design of #18950

Since the aimed developers are Pulsar core developers, it's added in
the pulsar-common module (PulsarApi.proto is also in this module), not
the pulsar-client-api module.

The reason to made such changes in the previous PR was that TopicMessageId#create now cannot be a MessageIdAdv if MessageIdAdv is not in the pulsar-client-api module. However, it can be avoided.

Modifications

  • Move the MessageIdAdv class to the pulsar-common module.
  • Implement the MessageIdAdv interface in TopicMessageIdImpl instead of TopicMessageId.Impl.
  • Create a TopicMessageIdImpl instance for TopicMessageId#create via the DefaultImplementation class with the overhead of reflection.

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: BewareMyPower#27

### Motivation

apache#19414 does not follow the design
of apache#18950

> Since the aimed developers are Pulsar core developers, it's added in
> the pulsar-common module (PulsarApi.proto is also in this module), not
> the pulsar-client-api module.

The reason is that `TopicMessageId#create` now cannot be a
`MessageIdAdv` if `MessageIdAdv` is not in the `pulsar-client-api`
module.

### Modifications

- Move the `MessageIdAdv` class to the `pulsar-common` module.
- Implement the `MessageIdAdv` interface in `TopicMessageIdImpl` instead
  of `TopicMessageId.Impl`.
- Create a `TopicMessageIdImpl` instance for `TopicMessageId#create` via
  the `DefaultImplementation` class with the overhead of reflection.
@BewareMyPower BewareMyPower added the type/bug The PR fixed a bug or issue reported a bug label Apr 19, 2023
@BewareMyPower BewareMyPower added this to the 3.0.0 milestone Apr 19, 2023
@BewareMyPower BewareMyPower self-assigned this Apr 19, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 19, 2023
@cbornet
Copy link
Contributor

cbornet commented Apr 19, 2023

We are in code freeze for 3.0.
Since this is affecting the public API, I guess we want to have it to avoid a breaking change later ?
@BewareMyPower can you confirm or can this be delayed to 3.1 ?

@BewareMyPower
Copy link
Contributor Author

I guess we want to have it to avoid a breaking change later ?

Yes. It's important to include this change in the 3.0.0 release so I added the 3.0.0 milestone. Otherwise, this change could be a terrible breaking change.

BTW, I have also replied in the 3.0.0 candidate 2 verify mail list: https://lists.apache.org/thread/5bwg9dpcffnzvvgxc9dj1n44otmy81pf

@codecov-commenter
Copy link

Codecov Report

Merging #20139 (00c28e0) into master (9b72302) will increase coverage by 39.77%.
The diff coverage is 78.78%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20139       +/-   ##
=============================================
+ Coverage     33.17%   72.95%   +39.77%     
- Complexity    12236    31945    +19709     
=============================================
  Files          1499     1868      +369     
  Lines        114413   138418    +24005     
  Branches      12431    15236     +2805     
=============================================
+ Hits          37962   100977    +63015     
+ Misses        71499    29415    -42084     
- Partials       4952     8026     +3074     
Flag Coverage Δ
inttests 24.22% <45.45%> (?)
systests 24.75% <45.45%> (?)
unittests 72.25% <78.78%> (+39.07%) ⬆️

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

Impacted Files Coverage Δ
...nt/internal/PulsarClientImplementationBinding.java 62.50% <ø> (+12.50%) ⬆️
...ava/org/apache/pulsar/client/api/MessageIdAdv.java 84.21% <ø> (ø)
...nt/impl/PulsarClientImplementationBindingImpl.java 80.00% <28.57%> (+24.82%) ⬆️
.../apache/pulsar/client/impl/TopicMessageIdImpl.java 69.23% <90.00%> (+6.73%) ⬆️
...a/org/apache/pulsar/client/api/TopicMessageId.java 33.33% <100.00%> (-46.67%) ⬇️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 76.83% <100.00%> (+21.65%) ⬆️
...a/org/apache/pulsar/client/impl/MessageIdImpl.java 90.27% <100.00%> (+51.38%) ⬆️
...rg/apache/pulsar/client/impl/TopicMessageImpl.java 65.11% <100.00%> (+25.58%) ⬆️
...a/org/apache/pulsar/websocket/ConsumerHandler.java 67.51% <100.00%> (+67.51%) ⬆️

... and 1533 files with indirect coverage changes

@BewareMyPower BewareMyPower merged commit 99a68e4 into apache:master Apr 20, 2023
BewareMyPower added a commit that referenced this pull request Apr 20, 2023
@BewareMyPower
Copy link
Contributor Author

I have cherry-picked this PR to branch-3.0. /cc @cbornet @RobertIndie

@BewareMyPower BewareMyPower deleted the bewaremypower/pip-229-module-change branch March 5, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants