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

Closed
wants to merge 2 commits into from

Conversation

BewareMyPower
Copy link
Owner

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.
  • To handle the instance created by TopicMessageId well, add MessageIdAdvUtils#convert to add an extra deserialization and serialization of MessageId.

@BewareMyPower BewareMyPower force-pushed the bewaremypower/pip-229-module-change branch from 32b171a to 7be139d Compare April 19, 2023 14:05
### 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.
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@9b72302). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #27   +/-   ##
=========================================
  Coverage          ?   72.94%           
  Complexity        ?    31934           
=========================================
  Files             ?     1868           
  Lines             ?   138417           
  Branches          ?    15236           
=========================================
  Hits              ?   100972           
  Misses            ?    29409           
  Partials          ?     8036           
Flag Coverage Δ
inttests 24.29% <0.00%> (?)
systests 24.85% <0.00%> (?)
unittests 72.22% <0.00%> (?)

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

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label May 21, 2023
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants