[improve][broker] Refactor update topic partitions endpoint. by mattisonchao · Pull Request #19166 · apache/pulsar

mattisonchao · 2023-01-10T04:46:20Z

Motivation

This PR follow-up #19086 to clean up complex logic and remove duplicate method invoke.

Modifications

Remove unnecessary topic owner redirect.
Refactor unreadable code.
Change unreasonable status code 406 to 422.

Verifying this change

Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

The REST endpoints

Documentation

doc-not-needed

codecov-commenter · 2023-01-10T05:07:23Z

Codecov Report

Merging #19166 (47c5015) into master (246c270) will increase coverage by 5.10%.
The diff coverage is 59.50%.

@@             Coverage Diff              @@
##             master   #19166      +/-   ##
============================================
+ Coverage     43.01%   48.12%   +5.10%     
+ Complexity    10167     9640     -527     
============================================
  Files           747      636     -111     
  Lines         72255    60083   -12172     
  Branches       7786     6271    -1515     
============================================
- Hits          31080    28913    -2167     
+ Misses        37577    28048    -9529     
+ Partials       3598     3122     -476

Flag	Coverage Δ
unittests	`48.12% <59.50%> (+5.10%)`	⬆️

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

Impacted Files	Coverage Δ
.../org/apache/pulsar/broker/admin/AdminResource.java	`64.96% <ø> (+0.04%)`	⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`58.84% <55.23%> (-0.34%)`	⬇️
...pache/pulsar/broker/admin/v1/PersistentTopics.java	`48.42% <75.00%> (+0.45%)`	⬆️
...pache/pulsar/broker/admin/v2/PersistentTopics.java	`72.27% <100.00%> (-2.14%)`	⬇️
.../org/apache/pulsar/broker/lookup/LookupResult.java	`56.66% <0.00%> (-20.00%)`	⬇️
...ulsar/broker/namespace/NamespaceEphemeralData.java	`68.18% <0.00%> (-18.19%)`	⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java	`54.12% <0.00%> (-13.77%)`	⬇️
...balance/impl/SimpleResourceAllocationPolicies.java	`48.57% <0.00%> (-5.72%)`	⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java	`50.00% <0.00%> (-5.32%)`	⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java	`56.16% <0.00%> (-3.62%)`	⬇️
... and 259 more

Technoboy- · 2023-01-18T03:22:53Z

-                final PulsarAdmin adminClient;
+                int brokerMaximumPartitionsPerTopic = pulsarService.getConfiguration()
+                        .getMaxNumPartitionsPerPartitionedTopic();
+                if (brokerMaximumPartitionsPerTopic != 0 && expectPartitions > brokerMaximumPartitionsPerTopic) {


brokerMaximumPartitionsPerTopic != 0 -> brokerMaximumPartitionsPerTopic > 0
Seems better

…9166

…1067) ### Motivation There is two failed tests when using Pulsar 3.0.0: ``` === RUN TestUpdateTopicNotExist update_test.go:85: Error Trace: /pulsarctl/pkg/ctl/topic/update_test.go:85 Error: Not equal: expected: "code: 409 reason: Topic is not partitioned topic" actual : "code: 409 reason: Topic persistent://public/default/non-exist-topic is not the partitioned topic." Diff: --- Expected +++ Actual @@ -1 +1 @@ -code: 409 reason: Topic is not partitioned topic +code: 409 reason: Topic persistent://public/default/non-exist-topic is not the partitioned topic. Test: TestUpdateTopicNotExist --- FAIL: TestUpdateTopicNotExist (0.01s) === RUN TestUpdateNonPartitionedTopic update_test.go:96: Error Trace: /pulsarctl/pkg/ctl/topic/update_test.go:96 Error: Not equal: expected: "code: 409 reason: Topic is not partitioned topic" actual : "code: 409 reason: Topic persistent://public/default/test-update-non-partitioned-topic is not the partitioned topic." Diff: --- Expected +++ Actual @@ -1 +1 @@ -code: 409 reason: Topic is not partitioned topic +code: 409 reason: Topic persistent://public/default/test-update-non-partitioned-topic is not the partitioned topic. Test: TestUpdateNonPartitionedTopic --- FAIL: TestUpdateNonPartitionedTopic (0.02s) ``` More details in https://github.com/streamnative/pulsarctl/actions/runs/4976702208/jobs/8905382299?pr=1064#step:3:1349 This is because we change the log message in Pulsar 3.0.0 by this PR: apache/pulsar#19166 ### Modifications * Fix the assert of the test

…1067) ### Motivation There is two failed tests when using Pulsar 3.0.0: ``` === RUN TestUpdateTopicNotExist update_test.go:85: Error Trace: /pulsarctl/pkg/ctl/topic/update_test.go:85 Error: Not equal: expected: "code: 409 reason: Topic is not partitioned topic" actual : "code: 409 reason: Topic persistent://public/default/non-exist-topic is not the partitioned topic." Diff: --- Expected +++ Actual @@ -1 +1 @@ -code: 409 reason: Topic is not partitioned topic +code: 409 reason: Topic persistent://public/default/non-exist-topic is not the partitioned topic. Test: TestUpdateTopicNotExist --- FAIL: TestUpdateTopicNotExist (0.01s) === RUN TestUpdateNonPartitionedTopic update_test.go:96: Error Trace: /pulsarctl/pkg/ctl/topic/update_test.go:96 Error: Not equal: expected: "code: 409 reason: Topic is not partitioned topic" actual : "code: 409 reason: Topic persistent://public/default/test-update-non-partitioned-topic is not the partitioned topic." Diff: --- Expected +++ Actual @@ -1 +1 @@ -code: 409 reason: Topic is not partitioned topic +code: 409 reason: Topic persistent://public/default/test-update-non-partitioned-topic is not the partitioned topic. Test: TestUpdateNonPartitionedTopic --- FAIL: TestUpdateNonPartitionedTopic (0.02s) ``` More details in https://github.com/streamnative/pulsarctl/actions/runs/4976702208/jobs/8905382299?pr=1064#step:3:1349 This is because we change the log message in Pulsar 3.0.0 by this PR: apache/pulsar#19166 ### Modifications * Fix the assert of the test (cherry picked from commit 11a3e8f)

…19166) (cherry picked from commit 4d7c7d0) Signed-off-by: Zixuan Liu <nodeces@gmail.com>

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 10, 2023

mattisonchao requested review from Technoboy-, codelipenghui, coderzc, congbobo184 and eolivelli January 10, 2023 04:47

mattisonchao self-assigned this Jan 10, 2023

mattisonchao added the area/broker label Jan 10, 2023

mattisonchao added this to the 2.12.0 milestone Jan 10, 2023

mattisonchao added the ready-to-test label Jan 10, 2023

mattisonchao closed this Jan 10, 2023

mattisonchao reopened this Jan 10, 2023

mattisonchao requested a review from lhotari January 10, 2023 11:56

mattisonchao marked this pull request as draft January 12, 2023 05:15

mattisonchao added 12 commits January 16, 2023 09:04

[improve][broker] Refactor topic update partitions endpoint.

62c509b

Fix config check

f99d1fd

Fix test

46a733b

Fix checkstyle

d4b707d

Fix failed test

c1420a5

Fix test

450be8e

Revert some code

9c5aaa6

Cleanup

31359a9

Cleanup

12e9058

Cleanup

8f354e4

Cleanup

8c42da4

Cleanup

a3bf6ec

mattisonchao marked this pull request as ready for review January 17, 2023 09:47

mattisonchao added 2 commits January 17, 2023 17:50

Add subscription copying

89ca105

Fix unsupport api

ecd1670

mattisonchao added 2 commits January 17, 2023 18:06

Fix checkstyle

8726052

Fix problem

47c5015

Technoboy- reviewed Jan 18, 2023

View reviewed changes

Technoboy- approved these changes Jan 18, 2023

View reviewed changes

eolivelli approved these changes Jan 18, 2023

View reviewed changes

eolivelli merged commit 4d7c7d0 into apache:master Jan 18, 2023

mattisonchao deleted the refactor_update_partitions_endpoint branch January 18, 2023 08:37

nodece reviewed Mar 8, 2023

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

gaozhangmin pushed a commit to gaozhangmin/pulsar that referenced this pull request Apr 6, 2023

[improve][broker] Refactor update topic partitions endpoint. apache#1…

8097c52

…9166

RobertIndie mentioned this pull request May 16, 2023

fix: Fix TestUpdateTopicNotExist and TestUpdateNonPartitionedTopic streamnative/pulsarctl#1067

Merged

3 tasks

nodece mentioned this pull request Dec 11, 2023

[cleanup][broker] Remove internalGetListAsync and validatePartitionTopicUpdateAsync #21699

Merged

4 tasks

hanmz mentioned this pull request Apr 2, 2024

[fix][broker] Update topic partition failed when config maxNumPartitionsPerPartitionedTopic<0 #22397

Merged

4 tasks

nodece pushed a commit to nodece/pulsar that referenced this pull request Sep 6, 2024

[improve][broker] Refactor update topic partitions endpoint. (apache#…

68f470d

…19166) (cherry picked from commit 4d7c7d0) Signed-off-by: Zixuan Liu <nodeces@gmail.com>

TakaHiR07 mentioned this pull request Dec 5, 2025

[Bug] update partition process is not reasonable since branch-3.0 #25041

Open

3 tasks

Shawyeok mentioned this pull request Mar 9, 2026

[fix][broker] fix error process in admin.updateTopicPartition #25042

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Refactor update topic partitions endpoint.#19166

[improve][broker] Refactor update topic partitions endpoint.#19166
eolivelli merged 16 commits into
apache:masterfrom
mattisonchao:refactor_update_partitions_endpoint

mattisonchao commented Jan 10, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Jan 10, 2023 •

edited

Loading

Uh oh!

Technoboy- Jan 18, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

mattisonchao commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

codecov-commenter commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Technoboy- Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattisonchao commented Jan 10, 2023 •

edited

Loading

codecov-commenter commented Jan 10, 2023 •

edited

Loading