Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode #12066

Merged
merged 3 commits into from
Oct 12, 2021
Merged

[PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode #12066

merged 3 commits into from
Oct 12, 2021

Conversation

EronWright
Copy link
Contributor

@EronWright EronWright commented Sep 16, 2021 •
edited
Loading

Master Issue: #12040

Related: #1152

Motivation

It should be possible to use the advertisedListeners configuration property in Pulsar standalone mode. One reason is to support dev scenarios involving multiple listeners. Another is to make standalone mode more consistent, e.g. to avoid special cases like seen here.

Modifications

  • Fix NoopLoadManager to actually advertise the configured advertised listeners.
  • Don't always force the 'proxy mode' in the lookup response; check that the advertisedAddress is a loopback address.
  • Initialize the internal admin client to use the actual advertised address.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: yes

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

    This PR addresses an undocumented limitation.

@merlimat merlimat added this to the 2.9.0 milestone Sep 17, 2021
@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Sep 17, 2021
@merlimat merlimat added the doc-not-needed Your PR changes do not impact docs label Sep 17, 2021
eolivelli
eolivelli requested changes Sep 19, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this patch is part of a PIP that has not yet been approved.

Please do not merge.

It would be better to mark this patch as draft probably

@EronWright
Copy link
Contributor Author

@eolivelli that makes sense, though I must admit that this particular PR has no real connection. It is just a straight bug fix. Seem OK to you?

315157973
315157973 reviewed Sep 20, 2021
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java Outdated
.isRunningStandalone();
// url, if the advertised address is localhost (per PulsarStandaloneStarter).
ServiceConfiguration conf = pulsarService.getConfiguration();
boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In standalone mode, what if we set advertisedAddress to 127.0.0.1?
Or the domain name host is set on the machine, but it points to the local.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java has an API in order to understand if the address is local, for instance we should support also IPv6 localhost addresses

Copy link
Contributor Author

@EronWright EronWright Sep 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea @315157973 and @eolivelli, I'll use InetAddress::isLoopbackAddress to test whether the advertised address is a loopback address, in which case we'll force the proxy mode (standalone only).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code to use isLoopbackAddress against the resolved LookupData. The logic can be understood as "don't return a LookupData that is pointing to a loopback address; force a proxy mode in that case".

@eolivelli eolivelli changed the title [Issue 12040][broker] Improved multi-listener in standalone mode [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode Sep 20, 2021
@eolivelli
Copy link
Contributor

@EronWright

This is a new feature that should not go to 2.8 release line in my opinion.
We should stick to adding new features of such complexity only to major releases, otherwise we can introduce stability issues.

@eolivelli eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021
eolivelli
eolivelli approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PIP approved

LGTM

@eolivelli
Copy link
Contributor

there is a conflict, can you please resolve it ?

@codelipenghui codelipenghui modified the milestones: 2.10.0, 2.9.0 Oct 11, 2021
@EronWright
Merge remote-tracking branch 'origin/master' into ewright/pip95-stand…
afa0d52 
…alone

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java
@eolivelli eolivelli merged commit 2c09593 into apache:master Oct 12, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 14, 2021
Merge branch 'up/master' into website/branch-2.7.2-chapter-2
8b9e28c 
* up/master: (26 commits)
  [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128)
  [website][upgrade]feat: docs migration - Development (apache#12320)
  Update delete inactive topic configuration documentation (apache#12350)
  [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056)
  Added Debezium Source for MS SQL Server (apache#12256)
  Fix: flaky oracle tests (apache#12306)
  [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341)
  [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
  feat(cli): add restart command to pulsar-daemon (apache#12279)
  [client-tools] Remove redundant initial value (apache#12296)
  Make AuthenticationTokenTest to run on windows (apache#12329)
  [offload] fix FileSystemManagedLedgerOffloader can not cleanup outdated ledger data (apache#12309)
  [Doc]--Update contents for Pulsar adaptor for Apache Spark (apache#12338)
  [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode (apache#12066)
  [website][upgrade]feat: docs migration - Cookbooks (apache#12319)
  [testclient] Make --payload-file take effect in PerformanceClient (apache#12187)
  [website][upgrade]feat: docs migration - adaptor (apache#12318)
  [pulsar-client] Add partition-change api for producer/consumer interceptors (apache#12287)
  [Transaction]Fix lowWaterMark of TopicTransactionBuffer (apache#12312)
  [pulsar-admin] New option takes precedence over deprecated option (apache#12260)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versions.json
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@EronWright @ciaocloud
[PIP 95][Issue 12040][broker] Improved multi-listener in standalone m…
cf40b96 
…ode (apache#12066)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@315157973 315157973 315157973 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@merlimat merlimat merlimat approved these changes

@hangc0276 hangc0276 Awaiting requested review from hangc0276

Assignees

@EronWright EronWright

Labels
doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@EronWright @eolivelli @merlimat @hangc0276 @315157973 @codelipenghui