[fix][broker] pre-create non-partitioned system topics for load balance extension by heesung-sohn · Pull Request #20370 · apache/pulsar

heesung-sohn · 2023-05-23T04:23:11Z

PIP: #16691

Motivation

We need to create system topics without partitions explicitly. Currently, we do not support partitioned system topics.

Modifications

create system topics without partitions explicitly

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is already covered by existing tests, such as (please describe tests).

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

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository:

…ce extension

poorbarcode · 2023-05-23T08:09:38Z

@heesung-sn

Since this patch fixes a bug, can we add a test to reproduce it which the current PR fixed?

heesung-sohn · 2023-05-23T16:20:30Z

@heesung-sn

Since this patch fixes a bug, can we add a test to reproduce it which the current PR fixed?

conflictAndCompactionTest with this config covers this case.
ServiceUnitStateChannelTest.java

michaeljmarshall

We need to create system topics without partitions explicitly. Currently, we do not support partitioned system topics.

Is this documented anywhere? It makes sense to me, but it seems like an important implementation detail.

michaeljmarshall · 2023-05-23T17:22:57Z

        return config.isLoadBalancerDebugModeEnabled() || log.isDebugEnabled();
    }

+    public static void createSystemTopic(PulsarService pulsar, String topic) throws PulsarServerException {


Nit: why is this method public and static? It seems like it should be private since it is used only within an instance of this class.

This is used in ServiceUnitStateChannel, too, to create a system topic inside ServiceUnitStateChannel.

Oops, I missed that reference. I still worry about misuse of this method because it is public and static--it's probably not a large concern though.

heesung-sohn · 2023-05-23T19:47:33Z

We need to create system topics without partitions explicitly. Currently, we do not support partitioned system topics.

Is this documented anywhere? It makes sense to me, but it seems like an important implementation detail.

I added a comment in the pip, #16691 (comment)

This partitioned system topics has been scoped out in the first iteration(as we don't see strong demand for this), but to support really large clusters, we(or anybody) could easily expand the current behavior and support partitioned system topics with multi-leaders.

heesung-sohn · 2023-05-23T23:08:43Z

/pulsarbot rerun-failure-checks

heesung-sohn · 2023-05-24T03:21:37Z

/pulsarbot rerun-failure-checks

michaeljmarshall · 2023-05-24T03:31:52Z


+    public static void createSystemTopic(PulsarService pulsar, String topic) throws PulsarServerException {
+        try {
+            pulsar.getAdminClient().topics().createNonPartitionedTopic(topic);


What are the consequences of spurious failures here. Does an error lead the broker to need to restart?

Also, how does this work in a system where there is just one broker running in a new cluster? I have not kept up with the load balancing work, but is there any chance of deadlock because creating this topic early requires a leader to already be selected? If this is covered by other documentation, definitely just point me to it. Thanks

The broker needs to restart when an exception is thrown here.

We elect the leader before creating this system topic.

https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java#L239-L240

Thanks for confirming.

codecov-commenter · 2023-05-24T03:47:28Z

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.97%. Comparing base (548fd22) to head (11f217b).
⚠️ Report is 1980 commits behind head on master.

Files with missing lines	Patch %	Lines
...dbalance/extensions/ExtensibleLoadManagerImpl.java	81.81%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #20370       +/-   ##
=============================================
+ Coverage     36.89%   72.97%   +36.08%     
- Complexity    12053    31895    +19842     
=============================================
  Files          1687     1864      +177     
  Lines        128826   138397     +9571     
  Branches      14013    15184     +1171     
=============================================
+ Hits          47528   100999    +53471     
+ Misses        75040    29397    -45643     
- Partials       6258     8001     +1743

Flag	Coverage Δ
inttests	`24.16% <0.00%> (-0.01%)`	⬇️
systests	`24.98% <0.00%> (-0.19%)`	⬇️
unittests	`72.23% <83.33%> (+40.28%)`	⬆️

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

Files with missing lines	Coverage Δ
...xtensions/channel/ServiceUnitStateChannelImpl.java	`83.27% <100.00%> (+82.72%)`	⬆️
...dbalance/extensions/ExtensibleLoadManagerImpl.java	`78.67% <81.81%> (+76.29%)`	⬆️

... and 1430 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

michaeljmarshall · 2023-05-24T04:25:52Z


+    public static void createSystemTopic(PulsarService pulsar, String topic) throws PulsarServerException {
+        try {
+            pulsar.getAdminClient().topics().createNonPartitionedTopic(topic);


Thanks for confirming.

…ce extension (#20370) PIP: #16691 ### Motivation We need to create system topics without partitions explicitly. Currently, we do not support partitioned system topics. ### Modifications create system topics without partitions explicitly (cherry picked from commit 1080ad5)

[fix][broker] pre-create non-partitioned system topics for load balan…

806bf14

…ce extension

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

Demogorgon314 self-requested a review May 23, 2023 04:30

Demogorgon314 added the ready-to-test label May 23, 2023

poorbarcode approved these changes May 23, 2023

View reviewed changes

poorbarcode assigned heesung-sohn May 23, 2023

poorbarcode added this to the 3.1.0 milestone May 23, 2023

poorbarcode added the release/3.0.1 label May 23, 2023

mattisonchao approved these changes May 23, 2023

View reviewed changes

Demogorgon314 approved these changes May 23, 2023

View reviewed changes

michaeljmarshall reviewed May 23, 2023

View reviewed changes

fix unused import

11f217b

michaeljmarshall reviewed May 24, 2023

View reviewed changes

heesung-sohn mentioned this pull request May 24, 2023

PIP-192: New Pulsar Broker Load Balancer #16691

Closed

michaeljmarshall approved these changes May 24, 2023

View reviewed changes

michaeljmarshall merged commit 1080ad5 into apache:master May 24, 2023

michaeljmarshall mentioned this pull request May 24, 2023

Flaky-test: AdminApi2Test.resetClusters #20376

Open

2 tasks

lhotari mentioned this pull request May 25, 2023

[improve][broker] Create non-partitioned system topics in auto topic creation #20397

Open

4 tasks

poorbarcode added the cherry-picked/branch-3.0 label May 30, 2023

michaeljmarshall mentioned this pull request Jun 20, 2023

[fix][broker] new load balancer system topic should not be auto-created now #20566

Merged

15 tasks

RobertIndie added the area/broker label Aug 7, 2023

heesung-sohn deleted the pip-192-topic-pre-create branch April 2, 2024 17:45

Uh oh!

Conversation

heesung-sohn commented May 23, 2023

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

poorbarcode commented May 23, 2023

Uh oh!

heesung-sohn commented May 23, 2023

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaeljmarshall May 23, 2023

Choose a reason for hiding this comment

Uh oh!

heesung-sohn May 23, 2023

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall May 24, 2023

Choose a reason for hiding this comment

Uh oh!

heesung-sohn commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heesung-sohn commented May 23, 2023

Uh oh!

heesung-sohn commented May 24, 2023

Uh oh!

michaeljmarshall May 24, 2023

Choose a reason for hiding this comment

Uh oh!

heesung-sohn May 24, 2023

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall May 24, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

michaeljmarshall May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

heesung-sohn commented May 23, 2023 •

edited

Loading

codecov-commenter commented May 24, 2023 •

edited

Loading