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.

Sign up for GitHub

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

Jump to bottom

[ISSUE #7543] Retry topic v2 in pop #7544

Merged
merged 7 commits into from Nov 23, 2023
Merged

[ISSUE #7543] Retry topic v2 in pop #7544

merged 7 commits into from Nov 23, 2023

Conversation

drpmma
Copy link
Contributor

@drpmma drpmma commented Nov 7, 2023

Which Issue(s) This PR Fixes

Fixes #7543

Brief Description

Use new retry topic to solve pop retry topic message notification and split group from retry topic.

How Did You Test This Change?

Unit test.

@xdkxlk
Copy link
Contributor

xdkxlk commented Nov 7, 2023

We need to consider the modifications from #7208 in order to ensure compatibility.

lollipopjin
lollipopjin approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@lollipopjin lollipopjin left a comment

Choose a reason for hiding this comment

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

LGTM

joeCarf
joeCarf reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@joeCarf joeCarf left a comment

Choose a reason for hiding this comment

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

We need to consider the modifications from #7208 in order to ensure compatibility.

+1， it will only delete the v2 topic instead of the v1 topic

RongtongJin
RongtongJin previously approved these changes Nov 8, 2023
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023 •
edited

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (48ef5ce) 43.01% compared to head (76c16aa) 43.14%.
Report is 15 commits behind head on develop.

Files Patch % Lines
...rocketmq/broker/processor/PopMessageProcessor.java 13.33% 11 Missing and 2 partials ⚠️
...rocketmq/broker/metrics/ConsumerLagCalculator.java 0.00% 8 Missing ⚠️
...in/java/org/apache/rocketmq/common/KeyBuilder.java 56.25% 2 Missing and 5 partials ⚠️
...etmq/broker/longpolling/PopLongPollingService.java 0.00% 5 Missing ⚠️
.../java/org/apache/rocketmq/common/BrokerConfig.java 25.00% 3 Missing ⚠️
...broker/filter/ExpressionForRetryMessageFilter.java 0.00% 1 Missing ⚠️
...cketmq/broker/processor/NotificationProcessor.java 0.00% 1 Missing ⚠️
...ocketmq/broker/processor/SendMessageProcessor.java 0.00% 1 Missing ⚠️
...s/command/consumer/ConsumerProgressSubCommand.java 0.00% 1 Missing ⚠️
.../apache/rocketmq/tools/monitor/MonitorService.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             develop    #7544      +/-   ##
=============================================
+ Coverage      43.01%   43.14%   +0.13%     
- Complexity      9685     9771      +86     
=============================================
  Files           1158     1162       +4     
  Lines          83977    84301     +324     
  Branches       10915    10948      +33     
=============================================
+ Hits           36122    36373     +251     
- Misses         43362    43409      +47     
- Partials        4493     4519      +26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drpmma
Copy link
Contributor Author

drpmma commented Nov 9, 2023

We need to consider the modifications from #7208 in order to ensure compatibility.

fixed

@leizhiyuan
Copy link
Contributor

leizhiyuan commented Nov 9, 2023 •
edited

Can the forward slash be used for folders in Linux? The "topic" of Pop ultimately ends up as a folder on the machine？

Or, in other words, is the design utilizing a multi-level directory to support this type of topic?

joeCarf
joeCarf approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@joeCarf joeCarf 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
Copy link
Contributor Author

drpmma commented Nov 23, 2023 •
edited

Can the forward slash be used for folders in Linux? The "topic" of Pop ultimately ends up as a folder on the machine？

Or, in other words, is the design utilizing a multi-level directory to support this type of topic?

Change the separator from '/' to ':' to prevent multi-level directory case.

@drpmma drpmma merged commit 5b43387 into apache:develop Nov 23, 2023
10 checks passed
lizhimins pushed a commit to lizhimins/rocketmq that referenced this pull request Nov 27, 2023
@drpmma @lizhimins
[ISSUE apache#7543] Retry topic v2 in pop (apache#7544)
6f56170 
* Implement pop retry topic v2

* Use pop retry topic v2 to notify the origin topic

* add parse group

* retry topic v2 compatibility

 * calculate consumer lag

 * delete retry topic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RongtongJin RongtongJin RongtongJin approved these changes

@lollipopjin lollipopjin lollipopjin approved these changes

@joeCarf joeCarf joeCarf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] Pop retry topic v2
7 participants
@drpmma @xdkxlk @codecov-commenter @leizhiyuan @RongtongJin @lollipopjin @joeCarf