You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/16 19:23:17 UTC
[GitHub] [pulsar] EronWright opened a new pull request #12066: [Issue 12040][broker] Improved multi-listener in standalone mode
EronWright opened a new pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066
Master Issue: #12040
### 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](https://github.com/streamnative/rop/commit/3e59300628704e296f9a97870c7250ff616747ef).
### 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 `localhost`.
- 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#issuecomment-922875764
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on a change in pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r712312488
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
Good idea @eolivelli, I'll use [InetAddress::isLoopbackAddress](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#isLoopbackAddress()) to test whether the advertised address is a loopback address, in which case we'll force the proxy mode (standalone only).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on pull request #12066: [Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#issuecomment-922603226
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on a change in pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r712413394
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #12066: [Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r711866093
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on a change in pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r712312488
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
Good idea @315157973 and @eolivelli, I'll use [InetAddress::isLoopbackAddress](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#isLoopbackAddress()) to test whether the advertised address is a loopback address, in which case we'll force the proxy mode (standalone only).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on a change in pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r712312488
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
Good idea @eolivelli, I'll use [InetAddress::isLoopbackAddress](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#isLoopbackAddress()) to test whether the advertised address is a loopback address, in which case we'll force the proxy mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #12066: [Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r711851423
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12066: [Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#discussion_r712090595
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -294,9 +295,10 @@ protected String internalGetNamespaceBundle(TopicName topicName) {
newAuthoritative, LookupType.Redirect, requestId, false));
} else {
// When running in standalone mode we want to redirect the client through the service
- // url, so that the advertised address configuration is not relevant anymore.
- boolean redirectThroughServiceUrl = pulsarService.getConfiguration()
- .isRunningStandalone();
+ // url, if the advertised address is localhost (per PulsarStandaloneStarter).
+ ServiceConfiguration conf = pulsarService.getConfiguration();
+ boolean isLocalhost = StringUtils.equals("localhost", conf.getAdvertisedAddress());
Review comment:
Java has an API in order to understand if the address is local, for instance we should support also IPv6 localhost addresses
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #12066: [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12066:
URL: https://github.com/apache/pulsar/pull/12066#issuecomment-938390340
there is a conflict, can you please resolve it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org