[improve][broker] PIP-192: Implement extensible load manager by Demogorgon314 · Pull Request #19102 · apache/pulsar

Demogorgon314 · 2022-12-29T07:56:17Z

PIP: #16691

Motivation

Implement extensible load manager.

Modifications

For the PIP-192, this PR adds ExtensibleLoadManagerImpl and unit tests.

This PR also changes:

Added CompletableFuture<Optional<LookupResult>> findBrokerServiceUrl( Optional<ServiceUnitId> topic, ServiceUnitId bundle) to LoadManager interface.
Added CompletableFuture<Boolean> checkOwnershipAsync(Optional<ServiceUnitId> topic, ServiceUnitId bundle) to LoadManager interface.
Change CompletableFuture<String> getOwnerAsync(String serviceUnit) to CompletableFuture<Optional<String>> getOwnerAsync(String serviceUnit) to unify the result.

Documentation

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

Matching PR in forked repository

PR in forked repository: Demogorgon314#10

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LoadManager.java

...broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryTest.java

...in/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannel.java

heesung-sohn · 2022-12-30T01:11:35Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

I was working on these NamespaceService change as per pluggable new broker load balancer work.

Do you want to keep this change here in this PR?

I think we need to add NamespaceService unit tests to track this variation.

I'll add the units test to cover enable the extensible load manager case.

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java

heesung-sohn · 2022-12-30T21:22:56Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

 import org.apache.pulsar.broker.loadbalance.LeaderElectionService;
 import org.apache.pulsar.broker.loadbalance.LoadManager;
 import org.apache.pulsar.broker.loadbalance.ResourceUnit;
+import org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImpl;


It would be great to add unit tests in NamespaceServiceTest for these if-else variations. (assert if the new/old load manager is called for these Namespace public funcs).

Or, please add a TODO comment if we want to add such tests later.

if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(config)) { // new logic } else { // old logic }

Good point. I'll add it later.

After thinking about it, I guess adding tests later might be better since some methods of NamespaceService still need to add new logic when using extensible load manager.

nit: It would be great if we can have a separate PR to integrate ExtensibleLoadManagerImpl into the existing code(to the NamespaceService and PulsarService). This PR could be ExtensibleLoadManagerImpl (and other dependent changes) only.

I tried to split it into two PRs, but ExtensibleLoadManagerImpl test dependency topic lookup because we are using TableView in ServiceUnitStateChannel, so I have to implement the load manager function.

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java

...in/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannel.java

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java

heesung-sohn · 2023-01-10T17:36:05Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

 import org.apache.pulsar.broker.loadbalance.LeaderElectionService;
 import org.apache.pulsar.broker.loadbalance.LoadManager;
 import org.apache.pulsar.broker.loadbalance.ResourceUnit;
+import org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImpl;


nit: It would be great if we can have a separate PR to integrate ExtensibleLoadManagerImpl into the existing code(to the NamespaceService and PulsarService). This PR could be ExtensibleLoadManagerImpl (and other dependent changes) only.

eolivelli

+1

BewareMyPower · 2023-02-01T03:12:30Z

...broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java

@@ -0,0 +1,222 @@
+/*


Could you remove the BrokerRegistryImpl related code in this PR since I see they were added in #18810?

gaoran10

Great work! left some trivial comments.

gaoran10 · 2023-02-01T04:10:13Z

...broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java

@@ -0,0 +1,222 @@
+/*


+1, we'd better merge the PR #18810 first.

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java

…nager # Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java # pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java # pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryTest.java

…nager

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java

codecov-commenter · 2023-02-03T06:56:57Z

Codecov Report

Attention: Patch coverage is 8.69565% with 168 lines in your changes missing coverage. Please review.

Project coverage is 62.49%. Comparing base (60d8e64) to head (3684d5c).
Report is 1733 commits behind head on master.

Files with missing lines	Patch %	Lines
...dbalance/extensions/ExtensibleLoadManagerImpl.java	7.54%	98 Missing ⚠️
...lance/extensions/ExtensibleLoadManagerWrapper.java	0.00%	28 Missing ⚠️
...ache/pulsar/broker/namespace/NamespaceService.java	23.07%	14 Missing and 6 partials ⚠️
.../apache/pulsar/broker/loadbalance/LoadManager.java	0.00%	6 Missing ⚠️
...loadbalance/extensions/LoadManagerContextImpl.java	0.00%	6 Missing ⚠️
...xtensions/channel/ServiceUnitStateChannelImpl.java	0.00%	6 Missing ⚠️
...n/java/org/apache/pulsar/broker/PulsarService.java	33.33%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19102      +/-   ##
============================================
- Coverage     63.24%   62.49%   -0.76%     
+ Complexity    25936    25660     -276     
============================================
  Files          1823     1829       +6     
  Lines        133338   133915     +577     
  Branches      14677    14732      +55     
============================================
- Hits          84330    83690     -640     
- Misses        41290    42565    +1275     
+ Partials       7718     7660      -58

Flag	Coverage Δ
inttests	`24.85% <8.69%> (-0.04%)`	⬇️
systests	`25.68% <8.69%> (+0.01%)`	⬆️
unittests	`59.72% <8.69%> (-0.91%)`	⬇️

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

Files with missing lines	Coverage Δ
...ensions/strategy/LeastResourceUsageWithWeight.java	`0.00% <ø> (ø)`
...n/java/org/apache/pulsar/broker/PulsarService.java	`73.87% <33.33%> (+1.10%)`	⬆️
.../apache/pulsar/broker/loadbalance/LoadManager.java	`58.33% <0.00%> (-19.45%)`	⬇️
...loadbalance/extensions/LoadManagerContextImpl.java	`0.00% <0.00%> (ø)`
...xtensions/channel/ServiceUnitStateChannelImpl.java	`0.96% <0.00%> (-0.01%)`	⬇️
...ache/pulsar/broker/namespace/NamespaceService.java	`59.14% <23.07%> (-2.06%)`	⬇️
...lance/extensions/ExtensibleLoadManagerWrapper.java	`0.00% <0.00%> (ø)`
...dbalance/extensions/ExtensibleLoadManagerImpl.java	`7.54% <7.54%> (ø)`

... and 158 files with indirect coverage changes

lhotari · 2023-04-04T06:21:28Z

This PR introduced a new flaky test #20007 . @Demogorgon314 Do you have a chance to fix that?

asafm · 2024-02-18T15:14:28Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

+                    if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(config)) {
+                        return loadManager.get().findBrokerServiceUrl(Optional.of(topic), bundle);
+                    } else {
+                        // TODO: Add unit tests cover it.


My personal habit is to never add those TODOs - they never get done.

Thanks for your advice. We should refactor the NamespaceService instead of adding so many if else here. We can remove this to-do when we're doing refactoring.

Demogorgon314 self-assigned this Dec 29, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 29, 2022

Demogorgon314 force-pushed the Demogorgon314/Implement_extensible_load_manager branch from 5f44f74 to acc4788 Compare December 29, 2022 08:06

Demogorgon314 marked this pull request as ready for review December 29, 2022 08:12

Demogorgon314 force-pushed the Demogorgon314/Implement_extensible_load_manager branch from acc4788 to d48b52a Compare December 29, 2022 08:20

heesung-sohn reviewed Dec 30, 2022

View reviewed changes

Demogorgon314 force-pushed the Demogorgon314/Implement_extensible_load_manager branch from e9a9959 to 3d54c15 Compare December 30, 2022 03:53

heesung-sohn reviewed Dec 30, 2022

View reviewed changes

heesung-sohn mentioned this pull request Dec 31, 2022

PIP-192: New Pulsar Broker Load Balancer #16691

Closed

BewareMyPower reviewed Jan 4, 2023

View reviewed changes

BewareMyPower requested changes Jan 4, 2023

View reviewed changes

...in/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannel.java Outdated Show resolved Hide resolved

Demogorgon314 force-pushed the Demogorgon314/Implement_extensible_load_manager branch from ee5785f to cf0ad90 Compare January 4, 2023 10:07

BewareMyPower requested changes Jan 4, 2023

View reviewed changes

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java Show resolved Hide resolved

BewareMyPower requested changes Jan 4, 2023

View reviewed changes

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java Outdated Show resolved Hide resolved

Demogorgon314 requested review from BewareMyPower and heesung-sohn January 9, 2023 14:36

heesung-sohn approved these changes Jan 10, 2023

View reviewed changes

Demogorgon314 mentioned this pull request Jan 17, 2023

[improve][broker] PIP-192: Write the child ownership to ServiceUnitStateChannel instead of ZK when handling bundle split #18858

Merged

4 tasks

eolivelli approved these changes Jan 17, 2023

View reviewed changes

eolivelli added the ready-to-test label Jan 17, 2023

Demogorgon314 added 10 commits January 17, 2023 17:18

Implement broker registry for new load manager

5ccd54c

Add extensible load manager impl

da8c830

Update

a724902

Return empty broker when has broker filter exception

90422da

Do not copy broker candidates

ca074f9

Add todo comments

2e6149c

Change to Optional.empty() back

e512ca5

Use try finally

7210bd8

Create system namespace in test

5562d2f

Revert: Return empty broker when has broker filter exception

51be8fb

Add setNamespaceBundleAffinity method

3765839

Demogorgon314 force-pushed the Demogorgon314/Implement_extensible_load_manager branch from 6df4f47 to 3765839 Compare January 17, 2023 09:20

Fix compilation issue

e23a6a4

Demogorgon314 requested a review from shibd January 19, 2023 01:59

codelipenghui added this to the 3.0.0 milestone Jan 31, 2023

codelipenghui requested review from Technoboy- and gaoran10 February 1, 2023 02:08

BewareMyPower requested changes Feb 1, 2023

View reviewed changes

gaoran10 reviewed Feb 1, 2023

View reviewed changes

Demogorgon314 added 6 commits February 1, 2023 17:41

Merge master into current branch

fb156a5

Use set instead of collection

ef6d0cf

Merge branch 'master' into Demogorgon314/Implement_extensible_load_ma…

d31f27f

…nager

Fix test

053cd07

Remove synchronized modifier

0833b43

BewareMyPower reviewed Feb 2, 2023

View reviewed changes

...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java Show resolved Hide resolved

BewareMyPower approved these changes Feb 2, 2023

View reviewed changes

Close the load data store

b03c1bb

gaoran10 approved these changes Feb 2, 2023

View reviewed changes

Demogorgon314 added 4 commits February 2, 2023 15:50

Start leader election service before load manager start

9ac2d58

Fix test

070827b

Fix code style

b8af243

Use failed future

3684d5c

Demogorgon314 merged commit c3126c6 into apache:master Feb 3, 2023

asafm reviewed Feb 18, 2024

View reviewed changes

Conversation

Demogorgon314 commented Dec 29, 2022

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoran10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

lhotari commented Apr 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Comments

heesung-sohn Jan 10, 2023 •

edited

Loading

heesung-sohn Jan 10, 2023 •

edited

Loading

codecov-commenter commented Feb 3, 2023 •

edited

Loading