[fix][broker] Fix leader broker cannot be determined when the advertised address and advertised listeners are configured by lhotari · Pull Request #21894 · apache/pulsar

lhotari · 2024-01-13T01:15:12Z

Motivation

There's currently a problem that the leader broker cannot be determined when the advertised address and advertised listeners are configured.
The workaround to the problem is to properly configure an internal advertised listener which matches the advertised address. However, applying the workaround is brittle.

The code base has had inconsistent ways for a unique identifier for a broker. In some cases, there's a dummy "http://" prefix for the identifier even when the port is the https port. This is very confusing.
This PR makes the broker identifier match the "lookup service address" in all cases. The "lookup service address" has been also renamed in this PR to "broker id" since that is the true meaning of the previously called "lookup service address".

Modifications

consistently use the "broker id" as the broker's identifier in Pulsar load balancing.
- Get rid of the dummy "http://" prefix (which isn't a http url since it might point to a https port)
for backwards compatibility & mixed version support, add brokerId field to the leader broker information and keep the previous serviceUrl without changing it's meaning and content.

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#170

…igured

lhotari · 2024-01-13T01:29:05Z

bug report on Pulsar Slack, https://apache-pulsar.slack.com/archives/C5Z4T36F7/p1705041494476059
Waiting for the user to create an issue. In the meantime, error messages (to make this PR searchable with error messages):

05:19:57.541 [pulsar-2-5] WARN  org.apache.pulsar.broker.namespace.NamespaceService - The current leader broker LeaderBroker(serviceUrl=http://xx.yy.zz.xy:8080/) isn't active. Handling load manager decisions in a decentralized way. NamespaceBundle[tenant/ns/0x2fffffff_0x3269f69d]

...

05:19:55.176 [pulsar-2-6] WARN  org.apache.pulsar.broker.namespace.NamespaceService - Broker http://xx.yy.zz.xy:8080/ (xx.yy.zz.xy:8080) couldn't be found in available brokers prod-xx-1-pulsar-broker-10:8080,prod-xx-1-pulsar-broker-yy-7:8080,prod-xx-1-pulsar-broker-zz-5:8080,prod-xx-1-pulsar-broker-7:8080,prod-xx-1-pulsar-broker-zz-2:8080,prod-xx-1-pulsar-broker-yy-1:8080,prod-xx-1-pulsar-broker-yy-4:8080,prod-xx-1-pulsar-broker-2:8080,prod-xx-1-pulsar-broker-12:8080,prod-xx-1-pulsar-broker-yy-2:8080,prod-xx-1-pulsar-broker-15:8080

UPDATE: reported as #21897

codecov-commenter · 2024-01-13T02:37:43Z

Codecov Report

Attention: Patch coverage is 75.25253% with 49 lines in your changes missing coverage. Please review.

Project coverage is 73.60%. Comparing base (c834feb) to head (96fe11b).
Report is 1277 commits behind head on master.

Files with missing lines	Patch %	Lines
...sar/broker/loadbalance/impl/LoadManagerShared.java	51.21%	17 Missing and 3 partials ⚠️
...rg/apache/pulsar/broker/web/PulsarWebResource.java	60.86%	6 Missing and 3 partials ⚠️
...apache/pulsar/broker/loadbalance/LeaderBroker.java	14.28%	5 Missing and 1 partial ⚠️
...ache/pulsar/broker/namespace/NamespaceService.java	79.31%	4 Missing and 2 partials ⚠️
...che/pulsar/broker/loadbalance/NoopLoadManager.java	60.00%	2 Missing ⚠️
.../loadbalance/extensions/manager/UnloadManager.java	77.77%	0 Missing and 2 partials ⚠️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	80.00%	1 Missing ⚠️
...ce/extensions/reporter/BrokerLoadDataReporter.java	80.00%	0 Missing and 1 partial ⚠️
...extensions/reporter/TopBundleLoadDataReporter.java	80.00%	0 Missing and 1 partial ⚠️
...broker/loadbalance/impl/SimpleLoadManagerImpl.java	94.11%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #21894      +/-   ##
============================================
- Coverage     73.72%   73.60%   -0.12%     
- Complexity    32405    32406       +1     
============================================
  Files          1859     1861       +2     
  Lines        138273   138592     +319     
  Branches      15158    15182      +24     
============================================
+ Hits         101939   102015      +76     
- Misses        28488    28686     +198     
- Partials       7846     7891      +45

Flag	Coverage Δ
inttests	`24.12% <22.22%> (-0.16%)`	⬇️
systests	`23.65% <19.19%> (-0.25%)`	⬇️
unittests	`72.90% <73.23%> (-0.10%)`	⬇️

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

Files with missing lines	Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java	`82.31% <100.00%> (+0.16%)`	⬆️
...g/apache/pulsar/broker/admin/impl/BrokersBase.java	`70.86% <100.00%> (+0.25%)`	⬆️
...lsar/broker/loadbalance/LeaderElectionService.java	`100.00% <100.00%> (ø)`
...ker/loadbalance/extensions/BrokerRegistryImpl.java	`84.76% <100.00%> (ø)`
...dbalance/extensions/ExtensibleLoadManagerImpl.java	`79.56% <100.00%> (-0.40%)`	⬇️
...xtensions/channel/ServiceUnitStateChannelImpl.java	`84.39% <100.00%> (-0.07%)`	⬇️
...e/extensions/policies/IsolationPoliciesHelper.java	`81.81% <100.00%> (ø)`
...roker/loadbalance/impl/ModularLoadManagerImpl.java	`84.65% <100.00%> (-0.20%)`	⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java	`94.44% <100.00%> (+3.14%)`	⬆️
...rg/apache/pulsar/broker/service/BrokerService.java	`81.09% <ø> (-0.16%)`	⬇️
... and 14 more

... and 82 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.

dragosvictor

Nice fix! These URLs were always confusing.

Left a couple of comments below.

lhotari · 2024-01-14T18:49:31Z

Nice fix! These URLs were always confusing.

Left a couple of comments below.

Thanks for the review and suggestions. I made the suggested changes.

lhotari · 2024-01-14T18:49:53Z

There's also a test case that reproduces the issue.

dragosvictor

LGTM, thanks!

Meet0861 · 2024-01-18T07:21:56Z

@lhotari can this be cherry-picked in 2.10.4 and 2.11 also please

lhotari · 2024-01-18T08:25:03Z

@lhotari can this be cherry-picked in 2.10.4 and 2.11 also please

@Meet0861 I don't think that a cherry-pick patch could be applied cleanly. A backport would be needed.

lhotari · 2024-01-19T19:40:07Z

There seems to be 2 other patches that are related and might be needed when backporting to older branches: #21015 and #21633 .

…sed address and advertised listeners are configured (#21894) (cherry picked from commit 3158fd3)

lhotari · 2024-01-26T22:51:38Z

@lhotari can this be cherry-picked in 2.10.4 and 2.11 also please

@Meet0861 I don't think that a cherry-pick patch could be applied cleanly. A backport would be needed.

@Meet0861 The support for 2.10.x and 2.11.x has ended according to the Apache Pulsar Release Policy. There's a mailing list discussion about making an explicit decision about 2.10 and 2.11 . Please participate in the discussion.

…sed address and advertised listeners are configured (#21894) (cherry picked from commit 3158fd3)

…sed address and advertised listeners are configured (apache#21894) (cherry picked from commit 3158fd3) (cherry picked from commit 358d122)

[fix][broker] Fix leader election when the advertised address is conf…

ecd4ea5

…igured

lhotari added the type/bug The PR fixed a bug or issue reported a bug label Jan 13, 2024

lhotari added this to the 3.3.0 milestone Jan 13, 2024

lhotari requested review from Demogorgon314, Technoboy-, codelipenghui, coderzc, heesung-sohn and poorbarcode January 13, 2024 01:15

lhotari self-assigned this Jan 13, 2024

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

lhotari requested a review from merlimat January 13, 2024 01:22

lhotari added the ready-to-test label Jan 13, 2024

Add test for advertised listeners & leader election

daa7cec

dragosvictor reviewed Jan 13, 2024

View reviewed changes

lhotari added 2 commits January 14, 2024 20:32

Make all lookup calls concurrent

9a406a6

Apply suggestions made by Dragos

fa81bbc

lhotari requested a review from dragosvictor January 14, 2024 19:04

dragosvictor approved these changes Jan 15, 2024

View reviewed changes

poorbarcode requested a review from BewareMyPower January 15, 2024 04:13

Add test case using binary protocol for lookups

889a64e

mattisonchao reviewed Jan 15, 2024

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated

lhotari requested a review from mattisonchao January 15, 2024 14:52

heesung-sohn reviewed Jan 16, 2024

View reviewed changes

Comment thread ...roker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerWrapper.java

Comment thread ...ar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java Outdated

heesung-sohn approved these changes Jan 16, 2024

View reviewed changes

Technoboy- approved these changes Jan 17, 2024

View reviewed changes

lhotari added 2 commits January 18, 2024 10:17

Fix TransferShedderTest

e267aef

Simplify rule

048ce49

Fix ModularLoadManagerImplTest

96fe11b

lhotari merged commit 3158fd3 into apache:master Jan 19, 2024

lhotari mentioned this pull request Jan 19, 2024

[fix][broker] Fix PulsarService.getLookupServiceAddress returns wrong port if TLS is enabled #21015

Merged

4 tasks

This was referenced Jan 19, 2024

[Bug] PulsarService.getLookupServiceAddress returns wrong port if TLS is enabled #21012

Closed

[fix][broker] Restore the broker id to match the format used in existing Pulsar releases #21937

Merged

lhotari added a commit that referenced this pull request Jan 21, 2024

[fix][broker] Fix leader broker cannot be determined when the adverti…

940ede5

…sed address and advertised listeners are configured (#21894) (cherry picked from commit 3158fd3)

lhotari added the cherry-picked/branch-3.2 label Jan 21, 2024

lhotari modified the milestones: 3.3.0, 3.2.0 Jan 21, 2024

Technoboy- removed the cherry-picked/branch-3.2 label Jan 22, 2024

lhotari added a commit that referenced this pull request Jan 26, 2024

[fix][broker] Fix leader broker cannot be determined when the adverti…

358d122

…sed address and advertised listeners are configured (#21894) (cherry picked from commit 3158fd3)

lhotari added release/3.0.3 release/3.1.3 labels Jan 26, 2024

lhotari added a commit that referenced this pull request Jan 26, 2024

[fix][broker] Fix leader broker cannot be determined when the adverti…

7d28dbf

…sed address and advertised listeners are configured (#21894) (cherry picked from commit 3158fd3)

lhotari added cherry-picked/branch-3.0 cherry-picked/branch-3.1 labels Jan 26, 2024

lhotari mentioned this pull request Jan 30, 2024

[improve][broker] Do not close the socket if lookup failed due to LockBusyException #21993

Merged

4 tasks

lhotari mentioned this pull request Feb 13, 2024

[PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols #22039

Closed

15 tasks

Demogorgon314 mentioned this pull request Apr 11, 2024

[fix][broker] Check the broker is available for the SLA monitor bundle when the ExtensibleLoadManager is enabled #22485

Merged

5 tasks

This was referenced May 3, 2024

[improve][broker][WIP] Add brokerId to lookup results and load manager data #22647

Draft

fix the broker stats with different environments apache/pulsar-manager#570

Merged

lhotari mentioned this pull request Jun 26, 2024

[Bug][broker] BrokerId npe when broker restart #22975

Closed

3 tasks

Conversation

lhotari commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

lhotari commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dragosvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhotari commented Jan 14, 2024

Uh oh!

lhotari commented Jan 14, 2024

Uh oh!

dragosvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Meet0861 commented Jan 18, 2024

Uh oh!

lhotari commented Jan 18, 2024

Uh oh!

lhotari commented Jan 19, 2024

Uh oh!

lhotari commented Jan 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lhotari commented Jan 13, 2024 •

edited

Loading

lhotari commented Jan 13, 2024 •

edited

Loading

codecov-commenter commented Jan 13, 2024 •

edited

Loading