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 01:00:29 UTC
[GitHub] [pulsar] EronWright opened a new pull request #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
EronWright opened a new pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056
Master Issue: #12040
### Motivation
Add a new configuration setting `bindAddresses` to open additional server ports on the broker. Note that these are in addition to `brokerServicePort` / `brokerServicePortTls` for compatibility reasons.
Each new-style bind address is associated with an advertised listener, to be used as the default listener for topic lookup requests. See #12040 for details. A given listener may be associated with numerous bindings.
The scheme indicates the protocol handler and whether to use TLS on the server channel. This PR is focused on the Pulsar protocol handler, but it is anticipated that other protocols may be supported in future.
For example:
```
bindAddresses=external:pulsar://0.0.0.0:6652,external:pulsar+ssl://0.0.0.0:6653
bindAddress=0.0.0.0
brokerServicePort=6650
brokerServicePortTls=6651
advertisedListeners=cluster:pulsar://broker-1.local:6650,cluster:pulsar+ssl://broker-1.local:6651,external:pulsar://broker-1.example.dev:6652,external:pulsar+ssl://broker-1.example.dev:6653
internalListenerName=cluster
```
The above would produce three server sockets, with `6650` having no associated listener name (thus retaining existing lookup behavior of returning the internal listener), and with `6652` and `6653` having an association with listener name `external`. Given a lookup request on `6652` or `6653`, the `external` listener address would be returned.
### Modifications
- added configuration property `bindAddresses`
- implementing parsing and validation logic
- factored some utility code for formatting broker/web addresses
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
- [ ] Adhoc testing as outlined in PIP 95.
This change added tests and can be verified as follows:
- Added unit tests for configuration validation logic.
### 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: no
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [X] doc-required
--
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 #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-923361039
I've addressed the feedback, please take another look.
--
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] BewareMyPower commented on a change in pull request #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711567969
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
+ + "The Default value is 6650."
)
private Optional<Integer> brokerServicePort = Optional.of(6650);
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving tls secured binary protobuf requests"
+ doc = "The port for serving TLS-secured binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePortTls"
+ + "without an associated advertised listener."
)
Review comment:
Maybe we need to add space or newline for each line?
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640416
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
Review comment:
There is no strong association for `brokerServicePort` to a specific listener, and so the current behavior is in effect. The current behavior is that lookups use the internal listener unless the lookup request contains a `listenerName` parameter. Of course, the `listenerName` parameter is supported by the new bindings too, but they have a better default.
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640416
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
Review comment:
Yes. There is no strong association for `brokerServicePort` to a specific listener, and so the current behavior is in effect. The current behavior is that lookups use the internal listener unless the lookup request contains a `listenerName` parameter. Of course, the `listenerName` parameter is supported by the new bindings too, but they have a better default.
I will tweak the javadoc to be more clear.
--
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] Anonymitaet commented on a change in pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r714407972
##########
File path: site2/docs/reference-configuration.md
##########
@@ -162,6 +162,7 @@ Pulsar brokers are responsible for handling incoming messages from producers, di
|exposeConsumerLevelMetricsInPrometheus|Whether to enable consumer level metrics.|false|
|jvmGCMetricsLoggerClassName|Classname of Pluggable JVM GC metrics logger that can log GC specific metrics.|N/A|
|bindAddress| Hostname or IP address the service binds on, default is 0.0.0.0. |0.0.0.0|
+|bindAddresses| Additional Hostname or IP addresses the service binds on: `listener_name:scheme://host:port,...`. ||
Review comment:
I have the same question here.
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640906
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/BindAddressValidator.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * 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.validator;
+
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.ArrayList;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.ServiceConfigurationUtils;
+import org.apache.pulsar.common.configuration.BindAddress;
+
+import static java.util.stream.Collectors.toList;
+
+/**
+ * Validates bind address configurations.
+ */
+public class BindAddressValidator {
Review comment:
I considered that, but there is no need to perform that check, because broker startup will fail in that case anyway. I imagine that one could create conflicts in a variety of ways. For example, would `0.0.0.0:443` and `<hostname>:443` be in conflict? For these reasons, I feel we need do nothing.
--
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] Anonymitaet commented on pull request #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-922173112
@EronWright thanks for your contribution!
One quick question: need to add the configuration and descriptions to Pulsar docs (https://pulsar.apache.org/docs/en/next/reference-configuration/)?
--
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] codelipenghui commented on pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-939874765
/pulsarbot run-failure-checks
--
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] Huanli-Meng commented on a change in pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r713969586
##########
File path: site2/docs/reference-configuration.md
##########
@@ -162,6 +162,7 @@ Pulsar brokers are responsible for handling incoming messages from producers, di
|exposeConsumerLevelMetricsInPrometheus|Whether to enable consumer level metrics.|false|
|jvmGCMetricsLoggerClassName|Classname of Pluggable JVM GC metrics logger that can log GC specific metrics.|N/A|
|bindAddress| Hostname or IP address the service binds on, default is 0.0.0.0. |0.0.0.0|
+|bindAddresses| Additional Hostname or IP addresses the service binds on: `listener_name:scheme://host:port,...`. ||
Review comment:
could this filed be added to both the **Broker** section and the **Standalone** section as you have updated the `broker.conf` and the `standalone.conf` files.
--
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] codelipenghui commented on a change in pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r713530238
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/BindAddressValidator.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * 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.validator;
+
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.ArrayList;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.ServiceConfigurationUtils;
+import org.apache.pulsar.common.configuration.BindAddress;
+
+import static java.util.stream.Collectors.toList;
+
+/**
+ * Validates bind address configurations.
+ */
+public class BindAddressValidator {
Review comment:
I see.
--
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] codelipenghui merged pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056
--
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 #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640008
##########
File path: conf/standalone.conf
##########
@@ -33,6 +33,9 @@ webServicePort=8080
# Hostname or IP address the service binds on, default is 0.0.0.0.
bindAddress=0.0.0.0
+# Extra bind addresses for protocol handlers: <listener_name>:<scheme>://<host>:<port>,[...]
+bindAddresses=
Review comment:
Thanks, I consider the phrase 'protocol handlers' to include the Pulsar binary protocol, but I understand that's not typical. I will amend the javadoc to be more clear.
I will apply the change to broker.conf.
The `bindAddress` continues to be used for the `webServicePort`,`brokerServicePort`, and `brokerServicePortTls` bindings. These new bindings in `bindAddresses` are supplemental to those bindings, for compatibility purposes.
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640008
##########
File path: conf/standalone.conf
##########
@@ -33,6 +33,9 @@ webServicePort=8080
# Hostname or IP address the service binds on, default is 0.0.0.0.
bindAddress=0.0.0.0
+# Extra bind addresses for protocol handlers: <listener_name>:<scheme>://<host>:<port>,[...]
+bindAddresses=
Review comment:
Thanks, I consider the phrase 'protocol handlers' to include the Pulsar binary protocol, but I understand that's not typical. I will amend the javadoc to be more clear.
I will apply the change to broker.conf.
The `bindAddress` continues to be used for the `webServicePort,`brokerServicePort`, and `brokerServicePortTls` bindings. These new bindings in `bindAddresses ` are supplemental to those bindings, for compatibility purposes.
--
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] codelipenghui commented on a change in pull request #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711585850
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
+ + "The Default value is 6650."
)
private Optional<Integer> brokerServicePort = Optional.of(6650);
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving tls secured binary protobuf requests"
+ doc = "The port for serving TLS-secured binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePortTls"
+ + "without an associated advertised listener."
Review comment:
Same as the above comment.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/BindAddressValidator.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * 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.validator;
+
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.ArrayList;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.ServiceConfigurationUtils;
+import org.apache.pulsar.common.configuration.BindAddress;
+
+import static java.util.stream.Collectors.toList;
+
+/**
+ * Validates bind address configurations.
+ */
+public class BindAddressValidator {
+
+ private static final Pattern bindAddress = Pattern.compile("(?<name>\\w+):(?<url>.+)$");
Review comment:
```suggestion
private static final Pattern BIND_ADDRESSES_PATTERN = Pattern.compile("(?<name>\\w+):(?<url>.+)$");
```
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/BindAddressValidator.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * 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.validator;
+
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.ArrayList;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.ServiceConfigurationUtils;
+import org.apache.pulsar.common.configuration.BindAddress;
+
+import static java.util.stream.Collectors.toList;
+
+/**
+ * Validates bind address configurations.
+ */
+public class BindAddressValidator {
+
+ private static final Pattern bindAddress = Pattern.compile("(?<name>\\w+):(?<url>.+)$");
+
+ /**
+ * Validate the configuration of `bindAddresses`.
+ * @param config the pulsar broker configure.
+ * @param schemes a filter on the schemes of the bind addresses, or null to not apply a filter.
+ * @return a list of bind addresses.
+ */
+ public static List<BindAddress> validateBindAddresses(ServiceConfiguration config, Collection<String> schemes) {
+ // migrate the existing configuration properties
+ List<BindAddress> addresses = migrateBindAddresses(config);
+
+ // parse the list of additional bind addresses
+ Arrays
+ .stream(StringUtils.split(StringUtils.defaultString(config.getBindAddresses()), ","))
+ .map(bindAddress::matcher)
+ .filter(Matcher::matches)
Review comment:
If users provided an invalid `bind address`, we should make a clear prompt to users and stop to start the broker.
For example, if `pulsar://xxxxx:6650` is used, we will use `pulsar` as the listener name.
or `xxx:yyy:pulsar://xxxxx:6650`, we will use `xxx` as the listener name.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/BindAddressValidator.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * 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.validator;
+
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.ArrayList;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.ServiceConfigurationUtils;
+import org.apache.pulsar.common.configuration.BindAddress;
+
+import static java.util.stream.Collectors.toList;
+
+/**
+ * Validates bind address configurations.
+ */
+public class BindAddressValidator {
Review comment:
It is better to add a check to avoid using the same bind port in the bindAddresses.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
Review comment:
The default port can also associate with one or more advertised listeners? The current behavior is users can create advertised listeners for one bind port.
##########
File path: conf/standalone.conf
##########
@@ -33,6 +33,9 @@ webServicePort=8080
# Hostname or IP address the service binds on, default is 0.0.0.0.
bindAddress=0.0.0.0
+# Extra bind addresses for protocol handlers: <listener_name>:<scheme>://<host>:<port>,[...]
+bindAddresses=
Review comment:
I think it is not only for the protocol handlers? Without the protocol handlers, Pulsar still can bind different addresses for the internal/external access requirement. Maybe describe as `Used to bind multiple addresses for the broker` is reasonable here?
And we should also apply the change in the broker.conf
Is it better to avoid using `bindAddresses` and `bindAddress` together?
--
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] Anonymitaet commented on a change in pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r714407972
##########
File path: site2/docs/reference-configuration.md
##########
@@ -162,6 +162,7 @@ Pulsar brokers are responsible for handling incoming messages from producers, di
|exposeConsumerLevelMetricsInPrometheus|Whether to enable consumer level metrics.|false|
|jvmGCMetricsLoggerClassName|Classname of Pluggable JVM GC metrics logger that can log GC specific metrics.|N/A|
|bindAddress| Hostname or IP address the service binds on, default is 0.0.0.0. |0.0.0.0|
+|bindAddresses| Additional Hostname or IP addresses the service binds on: `listener_name:scheme://host:port,...`. ||
Review comment:
I have the same questions here.
--
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] BewareMyPower commented on pull request #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-940014752
/pulsarbot run-failure-checks
--
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 #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-941067555
CI error is related to this patch
please fix
```
testMalformed(org.apache.pulsar.broker.validator.BindAddressValidatorTest) Time elapsed: 0.033 s <<< FAILURE!
java.lang.IllegalArgumentException: bindAddresses: malformed: internal:
at org.apache.pulsar.broker.validator.BindAddressValidator.lambda$validateBindAddresses$0(BindAddressValidator.java:56)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234
```)
at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
--
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 #12056: [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#issuecomment-942772130
I believe this PR is ready to merge. The intermittent test failure seem unrelated.
```
Run 2: TopicPoliciesTest.testPolicyOverwrittenByNamespaceLevel:1034 ยป ConditionTimeout
```
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711640416
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -121,13 +121,18 @@
private String configurationStoreServers;
@FieldContext(
category = CATEGORY_SERVER,
- doc = "The port for serving binary protobuf requests"
+ doc = "The port for serving binary protobuf requests."
+ + "If set, defines a server binding for bindAddress:brokerServicePort"
+ + "without an associated advertised listener."
Review comment:
Yes. There is no strong association for `brokerServicePort` to a specific listener, and so the current behavior is in effect. The current behavior is that lookups use the internal listener unless the lookup request contains a `listenerName` parameter. Of course, the `listenerName` parameter is supported by the new bindings too, but they have a better default.
--
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 #12056: [Issue 12040][broker] Multiple bind addresses for Pulsar protocol
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #12056:
URL: https://github.com/apache/pulsar/pull/12056#discussion_r711207387
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -398,39 +403,46 @@ public void start() throws Exception {
this.producerNameGenerator = new DistributedIdGenerator(pulsar.getCoordinationService(),
PRODUCER_NAME_GENERATOR_PATH, pulsar.getConfiguration().getClusterName());
- ServerBootstrap bootstrap = defaultServerBootstrap.clone();
-
ServiceConfiguration serviceConfig = pulsar.getConfiguration();
-
- bootstrap.childHandler(
- pulsarChannelInitFactory.newPulsarChannelInitializer(pulsar, false));
-
- Optional<Integer> port = serviceConfig.getBrokerServicePort();
- if (port.isPresent()) {
- // Bind and start to accept incoming connections.
- InetSocketAddress addr = new InetSocketAddress(pulsar.getBindAddress(), port.get());
+ List<BindAddress> bindAddresses = BindAddressValidator.validateBindAddresses(serviceConfig,
+ Arrays.asList("pulsar", "pulsar+ssl"));
+ String internalListenerName = serviceConfig.getInternalListenerName();
+
+ // create a channel for each bind address
+ if (bindAddresses.size() == 0) {
+ throw new IllegalArgumentException("At least one broker bind address must be configured");
+ }
+ for (BindAddress a : bindAddresses) {
+ InetSocketAddress addr = new InetSocketAddress(a.getAddress().getHost(), a.getAddress().getPort());
+ boolean isTls = "pulsar+ssl".equals(a.getAddress().getScheme());
+ PulsarChannelInitializer.PulsarChannelOptions opts = PulsarChannelInitializer.PulsarChannelOptions.builder()
+ .enableTLS(isTls)
+ .listenerName(a.getListenerName()).build();
+
+ ServerBootstrap b = defaultServerBootstrap.clone();
+ b.childHandler(
+ pulsarChannelInitFactory.newPulsarChannelInitializer(pulsar, opts));
try {
- listenChannel = bootstrap.bind(addr).sync().channel();
- log.info("Started Pulsar Broker service on {}", listenChannel.localAddress());
- } catch (Exception e) {
- throw new IOException("Failed to bind Pulsar broker on " + addr, e);
- }
- }
+ Channel ch = b.bind(addr).sync().channel();
+ listenChannels.add(ch);
+
+ // identify the primary channel. Note that the legacy bindings appear first and have no listener.
+ if (StringUtils.isBlank(a.getListenerName())
+ || StringUtils.equalsIgnoreCase(a.getListenerName(), internalListenerName)) {
+ if (this.listenChannel == null && !isTls) {
+ this.listenChannel = ch;
+ }
+ if (this.listenChannelTls == null && isTls) {
+ this.listenChannelTls = ch;
+ }
+ }
- Optional<Integer> tlsPort = serviceConfig.getBrokerServicePortTls();
- if (tlsPort.isPresent()) {
- ServerBootstrap tlsBootstrap = bootstrap.clone();
Review comment:
An oddity in the old code is that `bootstrap` is cloned here, though it has the non-TLS child handler attached already. I assume the handler is replaced. I would expect the `defaultServerBootstrap` to be cloned, rather than taking a clone of a clone. See new code.
--
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