You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "f1amingo (via GitHub)" <gi...@apache.org> on 2023/05/31 11:48:46 UTC
[GitHub] [rocketmq] f1amingo opened a new pull request, #6842: new feature: pop batch ack for pushConsumer
f1amingo opened a new pull request, #6842:
URL: https://github.com/apache/rocketmq/pull/6842
<!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
### Which Issue(s) This PR Fixes
<!-- Please ensure that the related issue has already been created, and [link this pull request to that issue using keywords](<https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>) to ensure automatic closure. -->
Fixes #6841
### Brief Description
<!-- Write a brief description for your pull request to help the maintainer understand the reasons behind your changes. -->
Currently, the POP consumer can pull up to 32 messages in batch, but after consuming one message, it needs to immediately ack once without achieving batching, which is inefficient and consumes bandwidth and CPU resources for both consumers and brokers.
To address this issue, we propose a solution of POP batch ack.
See the issue #6841 for more details.
### How Did You Test This Change?
<!-- In order to ensure the code quality of Apache RocketMQ, we expect every pull request to have undergone thorough testing. -->
1. Added a large number of unit tests and almost covered the code changes.
2. Scenario simulation, sending and receiving a specified number of messages to ensure that consumption does not repeat.
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] RongtongJin commented on pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin commented on PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842#issuecomment-1573038995
LGTM～
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842#issuecomment-1577049675
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#6842](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (48a13b4) into [develop](https://app.codecov.io/gh/apache/rocketmq/commit/f4439c971c935e2427853589f816fffdcc9c36b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f4439c9) will **increase** coverage by `0.15%`.
> The diff coverage is `70.98%`.
```diff
@@ Coverage Diff @@
## develop #6842 +/- ##
=============================================
+ Coverage 42.84% 43.00% +0.15%
- Complexity 8986 9102 +116
=============================================
Files 1105 1114 +9
Lines 78472 79078 +606
Branches 10227 10301 +74
=============================================
+ Hits 33620 34004 +384
- Misses 40636 40828 +192
- Partials 4216 4246 +30
```
| [Impacted Files](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...apache/rocketmq/remoting/protocol/RequestCode.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL1JlcXVlc3RDb2RlLmphdmE=) | `0.00% <ø> (ø)` | |
| [...cketmq/remoting/protocol/header/ExtraInfoUtil.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL2hlYWRlci9FeHRyYUluZm9VdGlsLmphdmE=) | `11.53% <0.00%> (-0.37%)` | :arrow_down: |
| [...cketmq/broker/processor/PopBufferMergeService.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL1BvcEJ1ZmZlck1lcmdlU2VydmljZS5qYXZh) | `29.85% <16.66%> (-0.45%)` | :arrow_down: |
| [...emoting/protocol/BitSetSerializerDeserializer.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL0JpdFNldFNlcmlhbGl6ZXJEZXNlcmlhbGl6ZXIuamF2YQ==) | `66.66% <66.66%> (ø)` | |
| [...ting/protocol/body/BatchAckMessageRequestBody.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL2JvZHkvQmF0Y2hBY2tNZXNzYWdlUmVxdWVzdEJvZHkuamF2YQ==) | `71.42% <71.42%> (ø)` | |
| [...rocketmq/broker/processor/AckMessageProcessor.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL0Fja01lc3NhZ2VQcm9jZXNzb3IuamF2YQ==) | `73.80% <71.65%> (+27.77%)` | :arrow_up: |
| [...ache/rocketmq/remoting/protocol/body/BatchAck.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL3Byb3RvY29sL2JvZHkvQmF0Y2hBY2suamF2YQ==) | `96.55% <96.55%> (ø)` | |
| [...a/org/apache/rocketmq/broker/BrokerController.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvQnJva2VyQ29udHJvbGxlci5qYXZh) | `43.57% <100.00%> (+0.10%)` | :arrow_up: |
| [...mq/broker/processor/PopInflightMessageCounter.java](https://app.codecov.io/gh/apache/rocketmq/pull/6842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL1BvcEluZmxpZ2h0TWVzc2FnZUNvdW50ZXIuamF2YQ==) | `75.34% <100.00%> (-0.66%)` | :arrow_down: |
... and [48 files with indirect coverage changes](https://app.codecov.io/gh/apache/rocketmq/pull/6842/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] f1amingo commented on pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "f1amingo (via GitHub)" <gi...@apache.org>.
f1amingo commented on PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842#issuecomment-1575958247
> I noticed that we already had some discussions regarding the nested Pop mode of PushConsumer. I suggest that we refrain from introducing further complexity to PushConsumer until we reach a consensus on this issue.
I have read the issue about removing the POP from `PushConsumer`, and in the long run, this will be a correct decision as it can make the semantic difference between the 4.0 heavyweight client and the 5.0 lightweight client more obvious.
So we plan to only keep the modifications on the Broker side, and the modifications on the 4.0 client side will be discarded and will be re-implemented in the 5.0 client later.
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] drpmma merged pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "drpmma (via GitHub)" <gi...@apache.org>.
drpmma merged PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "zhouxinyu (via GitHub)" <gi...@apache.org>.
zhouxinyu commented on PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842#issuecomment-1573349755
I noticed that we already had some discussions regarding the nested Pop mode of PushConsumer. I suggest that we refrain from introducing further complexity to PushConsumer until we reach a consensus on this issue.
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] drpmma commented on pull request #6842: [ISSUE #6841] new feature: pop batch ack for pushConsumer
Posted by "drpmma (via GitHub)" <gi...@apache.org>.
drpmma commented on PR #6842:
URL: https://github.com/apache/rocketmq/pull/6842#issuecomment-1573542328
There's an issue about whether to remove pushConsumer cause it's not firstly designed for pop.
https://github.com/apache/rocketmq/issues/6769
--
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@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org