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][broker] Don't allow creating a non-persistent partitioned topic with '-partition-' in name #23488

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hanmz
Copy link
Contributor

@hanmz hanmz commented Oct 21, 2024

Motivation

Currently, we can create a non-persistent partition topic contain -partition-. However, we cannot use it as a normal non-persistent partition topic. For example, when we use the partitioned-stats command, we will encounter errors.

Partitioned Topic Name should not contain '-partition-'
Reason: Partitioned Topic Name should not contain '-partition-'

Modifications

We should disable the creation of non-persistent partition topics contain -partition-.

Documentation

Verifying this change

All unit tests passed, not have any behavior change.

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 21, 2024
@lhotari
Copy link
Member

lhotari commented Oct 21, 2024

Thanks for the contribution, @hanmz. Would you be able to add a test case to cover this change?

@lhotari lhotari changed the title [fix][broker]Not allow to create non-persistent partitioned topic if topic name contain '-partition-'. [fix][broker] Don't allow creating a non-persistent partitioned topic with '-partition-' in name Oct 21, 2024
@hanmz
Copy link
Contributor Author

hanmz commented Oct 28, 2024

Thanks for the contribution, @hanmz. Would you be able to add a test case to cover this change?

OK, I've added a test case.

@Technoboy- Technoboy- added this to the 4.1.0 milestone Oct 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.15%. Comparing base (bbc6224) to head (fbe1f32).
Report is 704 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (bbc6224) and HEAD (fbe1f32). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (bbc6224) HEAD (fbe1f32)
unittests 2 0
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #23488       +/-   ##
=============================================
- Coverage     73.57%   32.15%   -41.42%     
+ Complexity    32624       86    -32538     
=============================================
  Files          1877     1800       -77     
  Lines        139502   139828      +326     
  Branches      15299    15388       +89     
=============================================
- Hits         102638    44962    -57676     
- Misses        28908    88289    +59381     
+ Partials       7956     6577     -1379     
Flag Coverage Δ
inttests 27.24% <100.00%> (+2.66%) ⬆️
systests 24.34% <0.00%> (+0.01%) ⬆️
unittests ?

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

Files with missing lines Coverage Δ
...he/pulsar/broker/admin/v2/NonPersistentTopics.java 29.20% <100.00%> (-32.27%) ⬇️

... and 1627 files with indirect coverage changes

@lhotari
Copy link
Member

lhotari commented Nov 5, 2024

Closing and reopening to trigger a new CI run

@lhotari lhotari closed this Nov 5, 2024
@lhotari lhotari reopened this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs release/4.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants