-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix][broker] Fix leader broker cannot be determined when the advertised address and advertised listeners are configured #21894
[fix][broker] Fix leader broker cannot be determined when the advertised address and advertised listeners are configured #21894
Conversation
bug report on Pulsar Slack, https://apache-pulsar.slack.com/archives/C5Z4T36F7/p1705041494476059
...
UPDATE: reported as #21897 |
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix! These URLs were always confusing.
Left a couple of comments below.
...ava/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LeaderBroker.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/SLAMonitoringTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiMultiBrokersTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/SimpleLoadManagerImplTest.java
Show resolved
Hide resolved
Thanks for the review and suggestions. I made the suggested changes. |
There's also a test case that reproduces the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
...roker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerWrapper.java
Show resolved
Hide resolved
...ar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
Outdated
Show resolved
Hide resolved
@lhotari can this be cherry-picked in 2.10.4 and 2.11 also please |
@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 (apache#21894) (cherry picked from commit 3158fd3) (cherry picked from commit 358d122)
…sed address and advertised listeners are configured (apache#21894) (cherry picked from commit 3158fd3) (cherry picked from commit 358d122)
Fixes #21897
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
brokerId
field to the leader broker information and keep the previousserviceUrl
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