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

[improve][broker] Follow up #19230 to tighten the validation scope #19234

Merged
merged 2 commits into from Jan 15, 2023
Merged

[improve][broker] Follow up #19230 to tighten the validation scope #19234

merged 2 commits into from Jan 15, 2023

Conversation

mattisonchao
Copy link
Member

Motivation

This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the -partition-{index} template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

Modifications

  • tighten the validation scope

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@mattisonchao mattisonchao self-assigned this Jan 15, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 15, 2023
@mattisonchao
Copy link
Member Author

Thanks to @yuruguo. Could you help to review it?

@mattisonchao mattisonchao added this to the 2.12.0 milestone Jan 15, 2023
@mattisonchao mattisonchao reopened this Jan 15, 2023
Copy link
Contributor

@yuruguo yuruguo left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2023

Codecov Report

Merging #19234 (f8b9739) into master (accae9f) will decrease coverage by 1.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19234      +/-   ##
============================================
- Coverage     48.17%   47.14%   -1.04%     
- Complexity     9695    10697    +1002     
============================================
  Files           633      713      +80     
  Lines         59922    69729    +9807     
  Branches       6251     7498    +1247     
============================================
+ Hits          28870    32874    +4004     
- Misses        27947    33169    +5222     
- Partials       3105     3686     +581     
Flag Coverage Δ
unittests 47.14% <0.00%> (-1.04%) ⬇️

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

Impacted Files Coverage Δ
...pache/pulsar/broker/admin/v2/PersistentTopics.java 72.48% <0.00%> (-2.33%) ⬇️
...ice/streamingdispatch/PendingReadEntryRequest.java 0.00% <0.00%> (-68.19%) ⬇️
...ervice/streamingdispatch/StreamingEntryReader.java 0.00% <0.00%> (-61.41%) ⬇️
...istentStreamingDispatcherSingleActiveConsumer.java 0.00% <0.00%> (-50.52%) ⬇️
...ersistentStreamingDispatcherMultipleConsumers.java 0.00% <0.00%> (-45.55%) ⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 54.00% <0.00%> (-28.00%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 61.11% <0.00%> (-16.67%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...rg/apache/pulsar/broker/lookup/v1/TopicLookup.java 60.00% <0.00%> (-13.34%) ⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java 61.94% <0.00%> (-7.75%) ⬇️
... and 113 more

@mattisonchao mattisonchao merged commit 246c270 into apache:master Jan 15, 2023
@mattisonchao mattisonchao deleted the follow_up_19230 branch January 15, 2023 11:29
mattisonchao added a commit that referenced this pull request Jan 16, 2023
…19234)

### Motivation
This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

### Modifications

- tighten the validation scope

(cherry picked from commit 246c270)
mattisonchao added a commit that referenced this pull request Jan 16, 2023
mattisonchao added a commit that referenced this pull request Jan 17, 2023
…19234)

### Motivation
This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

### Modifications

- tighten the validation scope

(cherry picked from commit 246c270)
liangyepianzhou pushed a commit that referenced this pull request Feb 25, 2023
…19234)

This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

- tighten the validation scope

(cherry picked from commit 246c270)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…ope (apache#19234)

This PR is following up apache#19230

As @yuruguo mentioned apache#19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

- tighten the validation scope

(cherry picked from commit 246c270)
(cherry picked from commit 2e3e9de)
@Technoboy-
Copy link
Contributor

Cherry-picked by #19839

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.

None yet

5 participants