[improve][broker] PIP-192: Define new load manager base interfaces #18084

Demogorgon314 · 2022-10-18T07:43:29Z

Master Issue: #16691

Motivation

We will start raising PRs to implement PIP-192, #16691

Modifications

The PR adds base classes for the new broker load balance project and does not integrate with the existing load balance logic. This PR should not impact the existing broker load balance behavior.

For the pip-192 project, this PR

defines the base interface under org.apache.pulsar.broker.loadbalance.extensible package.
defines this BrokerRegistry public interface and its expected behaviors.
defines BrokerFilter interfaces.
defines LoadDataReporter interfaces.
defines NamespaceBundleSplitStrategy interfaces.
defines LoadManagerScheduler interfaces.
defines NamespaceUnloadStrategy interfaces.
defines LoadDataStore interfaces.
defines ExtensibleLoadManager interfaces.
defines LoadManagerContext interfaces.
defines BrokerLoadData and BrokerLookupData data classes.

Documentation

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

Matching PR in forked repository

PR in forked repository: Demogorgon314#4

heesung-sn

Hi, this looks great to me.

I have some minor general comments.

let's trim some of the public functions in these interfaces if not used.
let's use generic terms, e.g. service unit instead of bundle(so that the functionality can be more "extensible").
let's try to use record classes as much as we can.

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/BrokerRegistry.java

...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/ExtensibleLoadManager.java

...r/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/data/TopBundlesLoadData.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/models/Split.java

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/store/LoadDataStore.java

...r/src/test/java/org/apache/pulsar/broker/loadbalance/extensible/data/BrokerLoadDataTest.java

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/data/BrokerLoadData.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/BrokerRegistry.java

heesung-sn · 2022-11-08T16:38:41Z

Hi, I am not sure pulsar follows this naming convention, but I think we can make a creative name for this project and use it as the package name, like org.apache.pulsar.broker.loadbalance.(project name). I propose “Overlord”. org.apache.pulsar.broker.loadbalance.overlord Origin: https://starcraft.fandom.com/wiki/Overlord I am also ok with org.apache.pulsar.broker.loadbalance.extensions or org.apache.pulsar.broker.loadbalance.extensible. Thanks, Heesung

…

On Tue, Nov 8, 2022 at 5:19 AM Kai Wang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/BrokerRegistry.java <#18084 (comment)>: > + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.loadbalance.extensible; I will more prefer to use org.apache.pulsar.broker.loadbalance.extensions or org.apache.pulsar.broker.loadbalance.extensible. @heesung-sn <https://github.com/heesung-sn> Do you have any other opinions? — Reply to this email directly, view it on GitHub <#18084 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYVJ675AMK74H72NFMJEDUTWHJHPTANCNFSM6AAAAAARHZDICI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

heesung-sn · 2022-11-08T16:49:48Z

Sorry, my last email got corrupted. Here is the corrected one. I am not sure pulsar follows this naming convention, but I think we can make a creative name for this project and use it as the package name, like org.apache.pulsar.broker.loadbalance.(project name). I propose “Overlord”. org.apache.pulsar.broker.loadbalance.overlord Origin: https://starcraft.fandom.com/wiki/Overlord I am also ok with org.apache.pulsar.broker.loadbalance.extensions or org.apache.pulsar.broker.loadbalance.extensible. Thanks, Heesung On Tue, Nov 8, 2022 at 8:38 AM Heesung Sohn ***@***.***> wrote:

…

Hi, I am not sure pulsar follows this naming convention, but I think we can make a creative name for this project and use it as the package name, like org.apache.pulsar.broker.loadbalance.(project name). I propose “Overlord”. org.apache.pulsar.broker.loadbalance.overlord Origin: https://starcraft.fandom.com/wiki/Overlord I am also ok with org.apache.pulsar.broker.loadbalance.extensions or org.apache.pulsar.broker.loadbalance.extensible. Thanks, Heesung On Tue, Nov 8, 2022 at 5:19 AM Kai Wang ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In > pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/BrokerRegistry.java > <#18084 (comment)>: > > > + * distributed with this work for additional information > + * regarding copyright ownership. The ASF licenses this file > + * to you under the Apache License, Version 2.0 (the > + * "License"); you may not use this file except in compliance > + * with the License. You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, > + * software distributed under the License is distributed on an > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > + * KIND, either express or implied. See the License for the > + * specific language governing permissions and limitations > + * under the License. > + */ > +package org.apache.pulsar.broker.loadbalance.extensible; > > I will more prefer to use org.apache.pulsar.broker.loadbalance.extensions > or org.apache.pulsar.broker.loadbalance.extensible. > > @heesung-sn <https://github.com/heesung-sn> Do you have any other > opinions? > > — > Reply to this email directly, view it on GitHub > <#18084 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AYVJ675AMK74H72NFMJEDUTWHJHPTANCNFSM6AAAAAARHZDICI> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

Demogorgon314 · 2022-11-09T07:21:01Z

@heesung-sn Use org.apache.pulsar.broker.loadbalance.overlord ok to me. But it looks like pulsar never used a creative name as the package name before. Not sure others develop agree with this.

eolivelli · 2022-11-09T21:16:02Z

Il Mer 9 Nov 2022, 08:21 Kai Wang ***@***.***> ha scritto:

@heesung-sn <https://github.com/heesung-sn> Use org.apache.pulsar.broker.loadbalance.overlord ok to me.

I am not sure. We are not used to this kind of creative names. We could do it but I prefer something that is meaningful for users. I would go with .extensions Enrico

…

— Reply to this email directly, view it on GitHub <#18084 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIHZNUSI4NFG67RF2GUNHLWHNGGRANCNFSM6AAAAAARHZDICI> . You are receiving this because you commented.Message ID: ***@***.***>

heesung-sn · 2022-11-09T22:14:12Z

ok. I am fine with `.extensions` too. Thanks, Heesung

…

On Wed, Nov 9, 2022 at 1:16 PM Enrico Olivelli ***@***.***> wrote: Il Mer 9 Nov 2022, 08:21 Kai Wang ***@***.***> ha scritto: > @heesung-sn <https://github.com/heesung-sn> Use > org.apache.pulsar.broker.loadbalance.overlord ok to me. > I am not sure. We are not used to this kind of creative names. We could do it but I prefer something that is meaningful for users. I would go with .extensions Enrico > — > Reply to this email directly, view it on GitHub > <#18084 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/ACIHZNUSI4NFG67RF2GUNHLWHNGGRANCNFSM6AAAAAARHZDICI > > . > You are receiving this because you commented.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#18084 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYVJ674I3LHUL2PZFJDNN73WHQIB3ANCNFSM6AAAAAARHZDICI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Demogorgon314 · 2022-11-14T04:14:42Z

Ping @eolivelli @codelipenghui @BewareMyPower @shibd

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/filter/BrokerFilter.java

BewareMyPower

It's better to add line breaks to all files. If you are using Intellij Idea, Go to Editor - General, click Ensure every saved file ends with a line break.

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java

codecov-commenter · 2022-11-18T02:46:35Z

Codecov Report

Merging #18084 (6534ce0) into master (7975023) will decrease coverage by 15.56%.
The diff coverage is 19.19%.

@@              Coverage Diff              @@
##             master   #18084       +/-   ##
=============================================
- Coverage     45.62%   30.05%   -15.57%     
+ Complexity    10075     6824     -3251     
=============================================
  Files           697      762       +65     
  Lines         68024    72901     +4877     
  Branches       7293     7812      +519     
=============================================
- Hits          31033    21911     -9122     
- Misses        33413    47978    +14565     
+ Partials       3578     3012      -566

Flag	Coverage Δ
unittests	`30.05% <19.19%> (-15.57%)`	⬇️

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

Impacted Files	Coverage Δ
...er/loadbalance/extensions/data/BrokerLoadData.java	`0.00% <0.00%> (ø)`
.../loadbalance/extensions/data/BrokerLookupData.java	`0.00% <0.00%> (ø)`
...oadbalance/extensions/data/TopBundlesLoadData.java	`0.00% <0.00%> (ø)`
...lance/extensions/store/LoadDataStoreException.java	`0.00% <0.00%> (ø)`
...nt/impl/PulsarClientImplementationBindingImpl.java	`72.41% <ø> (-0.47%)`	⬇️
...ar/client/impl/conf/ConsumerConfigurationData.java	`92.55% <ø> (+2.12%)`	⬆️
...he/pulsar/client/impl/TypedMessageBuilderImpl.java	`30.76% <59.37%> (+3.56%)`	⬆️
...n/java/org/apache/pulsar/client/api/RawReader.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pulsar/broker/admin/v1/Brokers.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pulsar/broker/admin/v2/Brokers.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 230 more

shibd · 2022-11-18T02:10:02Z

...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLookupData.java

+/**
+ * Defines the information required to broker lookup.
+ */
+public record BrokerLookupData (String webServiceUrl,


Refer PIP-192, this record should be LocalBrokerData?

IMO, BrokerLookupData maybe better since we already have a LocalBrokerData class (it contains lookup data and load data) in the old load manager impl. This record should only have lookup data.

The load data has split to BrokerLoadData.

Thanks for your explain. Maybe we can change PIP-192.

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java

shibd · 2022-11-18T02:43:55Z

...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManager.java

+     * @param serviceUnit service unit (e.g. bundle).
+     * @return Simple resource.
+     */
+    CompletableFuture<Optional<LookupResult>> assign(Optional<ServiceUnitId> topic, ServiceUnitId serviceUnit);


Who is to invoke this method? Should the method return BrokerLookupData? We should minimize coupling with the current implementation.

...ain/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/LoadManagerScheduler.java

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/LoadDataStore.java

shibd · 2022-11-18T03:10:31Z

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/LoadDataStore.java

+    /**
+     * Async push load data to store.
+     *
+     * @param key The load data key. (e.g. bundle)


What are the elements of a bundle? Should there be a namespace to distinguish?

Here is just an example. Actually, we always use the broker lookup service address as the key. I will remove this example, and explain it in impl class.

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java

heesung-sn · 2022-11-23T02:28:05Z

Lgtm

eolivelli

I have left only one major comment, apart from that I am +1 to this patch

eolivelli · 2022-11-23T08:22:26Z

.../org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceBundleSplitStrategy.java

+     * @param pulsarService Pulsar service to use.
+     * @return A set of the bundles that should be split.
+     */
+    Set<Split> findBundlesToSplit(LoadManagerContext context, PulsarService pulsarService);


why do we need to pass PulsarService pulsarService here ?
it is not an "interface" so we are leaking some "implementation details"

in any case....won't it be injected to implementations at construction time ?

eolivelli

LGTM

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistry.java

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java

codelipenghui · 2022-11-25T01:34:28Z

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java

+ */
+@Data
+public class BrokerLoadData {
+    private static final double gigaBitToByte = 128 * 1024 * 1024.0;


We have org.apache.pulsar.client.api.SizeUnit

I think we can add a method in SizeUnit

public double toBytes(double value) { return value * bytes; }

And use the SizeUnit directly.

I have removed some temporary unused methods from BrokerLoadData. I think we can add it when we use it.

Ping @codelipenghui

codelipenghui · 2022-11-25T01:36:11Z

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java

+    private static double maxWithinLimit(double limit, double...args) {
+        double max = 0.0;
+        for (double d : args) {
+            if (d > max && d <= limit) {
+                max = d;
+            }
+        }
+        return max;
+    }
+
+    private static double max(double...args) {
+        double max = Double.NEGATIVE_INFINITY;
+
+        for (double d : args) {
+            if (d > max) {
+                max = d;
+            }
+        }
+
+        return max;
+    }


Duplicated with LocalBrokerData

codelipenghui · 2022-11-25T01:38:00Z

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLoadData.java

+    public double getMaxResourceUsageWithExtendedNetworkSignal(ServiceConfiguration conf) {
+
+        double nicSpeedBytesInSec = getNicSpeedBytesInSec(conf);
+        return maxWithinLimit(100.0d,


And we can't apply the limitation of resource usage.
#18598 has provided more details.

pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java

…pache#18084) Master Issue: apache#16691 ### Motivation We will start raising PRs to implement PIP-192, apache#16691 ### Modifications The PR adds base classes for the new broker load balance project and does not integrate with the existing load balance logic. This PR should not impact the existing broker load balance behavior. For the pip-192 project, this PR * defines the base interface under `org.apache.pulsar.broker.loadbalance.extensible` package. * defines this `BrokerRegistry` public interface and its expected behaviors. * defines `BrokerFilter` interfaces. * defines `LoadDataReporter` interfaces. * defines `NamespaceBundleSplitStrategy` interfaces. * defines `LoadManagerScheduler` interfaces. * defines `NamespaceUnloadStrategy` interfaces. * defines `LoadDataStore` interfaces. * defines `ExtensibleLoadManager` interfaces. * defines `LoadManagerContext` interfaces. * defines `BrokerLoadData` and `BrokerLookupData` data classes.

Demogorgon314 self-assigned this Oct 18, 2022

Demogorgon314 changed the title ~~PIP-192: Define new load manager base interfaces~~ [improve][broker] PIP-192: Define new load manager base interfaces Oct 18, 2022

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

Demogorgon314 marked this pull request as ready for review October 18, 2022 11:30

heesung-sn reviewed Oct 18, 2022

View reviewed changes

heesung-sn approved these changes Oct 20, 2022

View reviewed changes

Demogorgon314 requested review from BewareMyPower, codelipenghui, hangc0276 and merlimat October 26, 2022 01:13

Demogorgon314 added type/feature The PR added a new feature or issue requested a new feature area/broker type/PIP labels Oct 26, 2022

eolivelli requested changes Nov 7, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensible/BrokerRegistry.java Outdated Show resolved Hide resolved

Demogorgon314 force-pushed the PIP-192-New_Load_Manager_Base_Classes branch from 5e7dad5 to 2458fbe Compare November 10, 2022 01:49

heesung-sn mentioned this pull request Nov 10, 2022

[improve][broker] PIP-192 Added ServiceUnitStateChannel interface #18079

Merged

14 tasks

Demogorgon314 force-pushed the PIP-192-New_Load_Manager_Base_Classes branch from 2458fbe to 07e8ab9 Compare November 14, 2022 04:13

Demogorgon314 added the ready-to-test label Nov 17, 2022

heesung-sn mentioned this pull request Nov 17, 2022

PIP-192: New Pulsar Broker Load Balancer #16691

Closed

BewareMyPower requested changes Nov 17, 2022

View reviewed changes

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/filter/BrokerFilter.java Outdated Show resolved Hide resolved

BewareMyPower reviewed Nov 17, 2022

View reviewed changes

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

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

Demogorgon314 force-pushed the PIP-192-New_Load_Manager_Base_Classes branch from 07e8ab9 to 5cb7b56 Compare November 18, 2022 02:27

shibd reviewed Nov 18, 2022

View reviewed changes

BewareMyPower requested changes Nov 21, 2022

View reviewed changes

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

Demogorgon314 requested review from eolivelli, heesung-sn and BewareMyPower and removed request for heesung-sn November 22, 2022 15:40

shibd approved these changes Nov 23, 2022

View reviewed changes

heesung-sn approved these changes Nov 23, 2022

View reviewed changes

eolivelli requested changes Nov 23, 2022

View reviewed changes

eolivelli approved these changes Nov 23, 2022

View reviewed changes

BewareMyPower reviewed Nov 23, 2022

View reviewed changes

BewareMyPower approved these changes Nov 23, 2022

View reviewed changes

codelipenghui reviewed Nov 25, 2022

View reviewed changes

BewareMyPower reviewed Nov 25, 2022

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LocalBrokerData.java Outdated Show resolved Hide resolved

Demogorgon314 requested a review from codelipenghui November 27, 2022 14:33

Demogorgon314 added 6 commits November 30, 2022 20:40

PIP-192: Define new load manager base interfaces

12d444d

Addressed reviewer's comments

02c5cff

Addressed reviewer's comments

993c882

Addressed reviewer's comments

1ac4f16

Use async interface

128d2b0

Remove temporary unused method

8e2240e

Demogorgon314 force-pushed the PIP-192-New_Load_Manager_Base_Classes branch from 6534ce0 to 8e2240e Compare November 30, 2022 12:41

codelipenghui approved these changes Dec 2, 2022

View reviewed changes

Demogorgon314 merged commit fd9444c into apache:master Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] PIP-192: Define new load manager base interfaces #18084

[improve][broker] PIP-192: Define new load manager base interfaces #18084

Demogorgon314 commented Oct 18, 2022

heesung-sn left a comment

heesung-sn commented Nov 8, 2022 via email

heesung-sn commented Nov 8, 2022 via email

Demogorgon314 commented Nov 9, 2022 •

edited

Loading

eolivelli commented Nov 9, 2022 via email

heesung-sn commented Nov 9, 2022 via email

Demogorgon314 commented Nov 14, 2022

BewareMyPower left a comment

codecov-commenter commented Nov 18, 2022 •

edited

Loading

shibd Nov 18, 2022

Demogorgon314 Nov 18, 2022

shibd Nov 18, 2022

shibd Nov 18, 2022

shibd Nov 18, 2022

Demogorgon314 Nov 18, 2022

heesung-sn commented Nov 23, 2022

eolivelli left a comment

eolivelli Nov 23, 2022

Demogorgon314 Nov 23, 2022

eolivelli left a comment

codelipenghui Nov 25, 2022

Demogorgon314 Nov 25, 2022

Demogorgon314 Dec 1, 2022

codelipenghui Nov 25, 2022

codelipenghui Nov 25, 2022

[improve][broker] PIP-192: Define new load manager base interfaces #18084

[improve][broker] PIP-192: Define new load manager base interfaces #18084

Conversation

Demogorgon314 commented Oct 18, 2022

Motivation

Modifications

Documentation

Matching PR in forked repository

heesung-sn left a comment

Choose a reason for hiding this comment

heesung-sn commented Nov 8, 2022 via email

heesung-sn commented Nov 8, 2022 via email

Demogorgon314 commented Nov 9, 2022 • edited Loading

eolivelli commented Nov 9, 2022 via email

heesung-sn commented Nov 9, 2022 via email

Demogorgon314 commented Nov 14, 2022

BewareMyPower left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heesung-sn commented Nov 23, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Demogorgon314 commented Nov 9, 2022 •

edited

Loading

codecov-commenter commented Nov 18, 2022 •

edited

Loading