You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2023/04/19 14:39:36 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request, #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
BewareMyPower opened a new pull request, #20139:
URL: https://github.com/apache/pulsar/pull/20139
### Motivation
https://github.com/apache/pulsar/pull/19414 does not follow the design of https://github.com/apache/pulsar/issues/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:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [x] 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
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/BewareMyPower/pulsar/pull/27
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#discussion_r1171990404
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicMessageIdImpl.java:
##########
@@ -18,15 +18,27 @@
*/
package org.apache.pulsar.client.impl;
+import java.util.BitSet;
import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.MessageIdAdv;
import org.apache.pulsar.client.api.TopicMessageId;
-public class TopicMessageIdImpl extends TopicMessageId.Impl {
+public class TopicMessageIdImpl implements MessageIdAdv, TopicMessageId {
- private final String topicName;
+ private final String ownerTopic;
+ private final MessageIdAdv msgId;
+ private final String topicName; // it's never used
+ public TopicMessageIdImpl(String topic, MessageIdAdv msgId) {
+ this.ownerTopic = topic;
+ this.msgId = msgId;
+ this.topicName = "";
+ }
Review Comment:
Nit:
Since we have a new constructor, the below one is deprecated.
We should use the new constructor in `MessageIdImpl#fromByteArrayWithTopic`
https://github.com/apache/pulsar/blob/6dc0b0ea38ab9dc410a24b19fd7567b9e013837d/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java#L144-L147
And if the challenge is the ownerTopic is different from topicName, we can try to get the topicName from the ownerTopic(partition name)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #20139:
URL: https://github.com/apache/pulsar/pull/20139
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#issuecomment-1516043025
I have cherry-picked this PR to branch-3.0. /cc @cbornet @RobertIndie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#discussion_r1172048436
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicMessageIdImpl.java:
##########
@@ -18,15 +18,27 @@
*/
package org.apache.pulsar.client.impl;
+import java.util.BitSet;
import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.MessageIdAdv;
import org.apache.pulsar.client.api.TopicMessageId;
-public class TopicMessageIdImpl extends TopicMessageId.Impl {
+public class TopicMessageIdImpl implements MessageIdAdv, TopicMessageId {
- private final String topicName;
+ private final String ownerTopic;
+ private final MessageIdAdv msgId;
+ private final String topicName; // it's never used
+ public TopicMessageIdImpl(String topic, MessageIdAdv msgId) {
+ this.ownerTopic = topic;
+ this.msgId = msgId;
+ this.topicName = "";
+ }
Review Comment:
I removed the references of the deprecated constructor now. The `topicName` field is never used. It's retained just for the deprecated getter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#issuecomment-1514936360
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "cbornet (via GitHub)" <gi...@apache.org>.
cbornet commented on PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#issuecomment-1514913172
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #20139: [fix][client] Move MessageIdAdv to the pulsar-common module
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20139:
URL: https://github.com/apache/pulsar/pull/20139#issuecomment-1515737697
## [Codecov](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20139](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (00c28e0) into [master](https://codecov.io/gh/apache/pulsar/commit/9b723022436cc1a150af765103e8d343679f92ce?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9b72302) will **increase** coverage by `39.77%`.
> The diff coverage is `78.78%`.
[](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nt/internal/PulsarClientImplementationBinding.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9jbGllbnQvaW50ZXJuYWwvUHVsc2FyQ2xpZW50SW1wbGVtZW50YXRpb25CaW5kaW5nLmphdmE=) | `62.50% <ø> (+12.50%)` | :arrow_up: |
| [...ava/org/apache/pulsar/client/api/MessageIdAdv.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9hcGkvTWVzc2FnZUlkQWR2LmphdmE=) | `84.21% <ø> (ø)` | |
| [...nt/impl/PulsarClientImplementationBindingImpl.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1B1bHNhckNsaWVudEltcGxlbWVudGF0aW9uQmluZGluZ0ltcGwuamF2YQ==) | `80.00% <28.57%> (+24.82%)` | :arrow_up: |
| [.../apache/pulsar/client/impl/TopicMessageIdImpl.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1RvcGljTWVzc2FnZUlkSW1wbC5qYXZh) | `69.23% <90.00%> (+6.73%)` | :arrow_up: |
| [...a/org/apache/pulsar/client/api/TopicMessageId.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9jbGllbnQvYXBpL1RvcGljTWVzc2FnZUlkLmphdmE=) | `33.33% <100.00%> (-46.67%)` | :arrow_down: |
| [...va/org/apache/pulsar/client/impl/ConsumerImpl.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVySW1wbC5qYXZh) | `76.83% <100.00%> (+21.65%)` | :arrow_up: |
| [...a/org/apache/pulsar/client/impl/MessageIdImpl.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL01lc3NhZ2VJZEltcGwuamF2YQ==) | `90.27% <100.00%> (+51.38%)` | :arrow_up: |
| [...rg/apache/pulsar/client/impl/TopicMessageImpl.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1RvcGljTWVzc2FnZUltcGwuamF2YQ==) | `65.11% <100.00%> (+25.58%)` | :arrow_up: |
| [...a/org/apache/pulsar/websocket/ConsumerHandler.java](https://codecov.io/gh/apache/pulsar/pull/20139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXdlYnNvY2tldC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL3dlYnNvY2tldC9Db25zdW1lckhhbmRsZXIuamF2YQ==) | `67.51% <100.00%> (+67.51%)` | :arrow_up: |
... and [1533 files with indirect coverage changes](https://codecov.io/gh/apache/pulsar/pull/20139/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org