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

[ISSUE #6841] new feature: pop batch ack for pushConsumer #6842

Merged
merged 1 commit into from
Jun 19, 2023
Merged

[ISSUE #6841] new feature: pop batch ack for pushConsumer #6842

merged 1 commit into from
Jun 19, 2023

Conversation

f1amingo
Copy link
Contributor

Which Issue(s) This PR Fixes

Fixes #6841

Brief Description

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?

  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.

@RongtongJin RongtongJin changed the title new feature: pop batch ack for pushConsumer [ISSUE #6841] new feature: pop batch ack for pushConsumer Jun 1, 2023
RongtongJin
RongtongJin previously approved these changes Jun 2, 2023
@RongtongJin
Copy link
Contributor

LGTM~

ferrirW
ferrirW previously approved these changes Jun 2, 2023
@zhouxinyu
Copy link
Member

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.

Copy link
Member

@zhouxinyu zhouxinyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for more discussions.

@drpmma
Copy link
Contributor

drpmma commented Jun 2, 2023

There's an issue about whether to remove pushConsumer cause it's not firstly designed for pop.

#6769

@f1amingo
Copy link
Contributor Author

f1amingo commented Jun 5, 2023

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.

@f1amingo f1amingo dismissed stale reviews from ferrirW and RongtongJin via 48a13b4 June 5, 2023 03:05
@f1amingo f1amingo requested a review from zhouxinyu June 5, 2023 09:27
@codecov-commenter
Copy link

Codecov Report

Merging #6842 (48a13b4) into develop (f4439c9) will increase coverage by 0.15%.
The diff coverage is 70.98%.

@@              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 Coverage Δ
...apache/rocketmq/remoting/protocol/RequestCode.java 0.00% <ø> (ø)
...cketmq/remoting/protocol/header/ExtraInfoUtil.java 11.53% <0.00%> (-0.37%) ⬇️
...cketmq/broker/processor/PopBufferMergeService.java 29.85% <16.66%> (-0.45%) ⬇️
...emoting/protocol/BitSetSerializerDeserializer.java 66.66% <66.66%> (ø)
...ting/protocol/body/BatchAckMessageRequestBody.java 71.42% <71.42%> (ø)
...rocketmq/broker/processor/AckMessageProcessor.java 73.80% <71.65%> (+27.77%) ⬆️
...ache/rocketmq/remoting/protocol/body/BatchAck.java 96.55% <96.55%> (ø)
...a/org/apache/rocketmq/broker/BrokerController.java 43.57% <100.00%> (+0.10%) ⬆️
...mq/broker/processor/PopInflightMessageCounter.java 75.34% <100.00%> (-0.66%) ⬇️

... and 48 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@xdkxlk xdkxlk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@drpmma drpmma merged commit 6238caa into apache:develop Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] pop batch ack for pushConsumer
7 participants