You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/26 12:35:57 UTC
[GitHub] [nifi] nabmoh123 opened a new pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
nabmoh123 opened a new pull request #4939:
URL: https://github.com/apache/nifi/pull/4939
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
This provides an enhancement to the AMQP Processors to allow multiple hosts to be specified in a broker list. By specifying multiple hosts, a connection is made to one of the hosts, errors are only thrown if none of the hosts in the list are reachable. This property should only be used to specify amqp nodes in the same cluster. By specifying 2 nodes from 2 different clusters, only one cluster will be interacted with by NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ Yes] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [Yes] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ Yes] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ Yes] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ Yes] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [Yes] Have you written or updated unit tests to verify your changes?
- [ Yes] Have you verified that the full build is successful on JDK 8?
- [ Yes] Have you verified that the full build is successful on JDK 11?
- [ N/A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ N/A] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ N/A] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ No, the other properties in the existing processor do not make use of a displayName ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ N/A] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nabmoh123 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
nabmoh123 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-814001461
> Thanks for this improvement. Could you add a customValidate method to check the configuration of the processor and make it invalid if both brokers and host properties are set simultaneously, or if none of the properties are set? You may find similar pieces of code in other processors.
>
> Right now, one could leave all properties empty, the processor would be valid and throw NPE exceptions since the `connection` object would not be set.
The Host and Port have default values set. So NiFi reenters the default values in if you leave the properties empty. Is it still worth adding in the customValidate for this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-814235193
I still think that a custom validate method makes sense. Especially to make the processor invalid if brokers, host, and port are all set. Because in that case it could confuse the users wrt to which properties are actually used. I acknowledge this is explained in the properties description but this would make things more "obvious".
We do that for some processors where it's possible to configure the same "thing" via multiple properties after we added some new properties over time and we kept the old ones to not break backward compatibility.
That's my only remaining comment, latest changes look good to me.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#discussion_r607015274
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java
##########
@@ -55,18 +57,26 @@
*/
abstract class AbstractAMQPProcessor<T extends AMQPWorker> extends AbstractProcessor {
+ public static final PropertyDescriptor BROKERS = new PropertyDescriptor.Builder()
+ .name("Brokers")
+ .description("A comma-separated list of known AMQP Brokers in the format <host>:<port> (e.g., localhost:5672). If this is " +
+ "set, Host Name and Port are ignored. Only include hosts from the same AMQP cluster.")
+ .required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.HOSTNAME_PORT_LIST_VALIDATOR)
+ .build();
public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
.name("Host Name")
- .description("Network address of AMQP broker (e.g., localhost)")
- .required(true)
+ .description("Network address of AMQP broker (e.g., localhost). If Brokers is set, then this property is ignored.")
+ .required(false)
.defaultValue("localhost")
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .addValidator(StandardValidators.NON_EMPTY_EL_VALIDATOR)
+ .addValidator(StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR)
Review comment:
Not sure about this change. Could you explain the intent?
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java
##########
@@ -317,7 +330,16 @@ public void handleUnexpectedConnectionDriverException(Connection conn, Throwable
});
try {
- Connection connection = cf.newConnection(executor);
+ Connection connection;
+ if (context.getProperty(BROKERS).isSet()) {
+ Address[] hostsList = createHostsList(context);
+ connection = cf.newConnection(executor, hostsList);
+ } else {
+ cf.setHost(context.getProperty(BROKERS).evaluateAttributeExpressions().getValue());
Review comment:
This should be ``HOST``, no?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nabmoh123 commented on a change in pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
nabmoh123 commented on a change in pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#discussion_r607722789
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java
##########
@@ -55,18 +57,26 @@
*/
abstract class AbstractAMQPProcessor<T extends AMQPWorker> extends AbstractProcessor {
+ public static final PropertyDescriptor BROKERS = new PropertyDescriptor.Builder()
+ .name("Brokers")
+ .description("A comma-separated list of known AMQP Brokers in the format <host>:<port> (e.g., localhost:5672). If this is " +
+ "set, Host Name and Port are ignored. Only include hosts from the same AMQP cluster.")
+ .required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.HOSTNAME_PORT_LIST_VALIDATOR)
+ .build();
public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
.name("Host Name")
- .description("Network address of AMQP broker (e.g., localhost)")
- .required(true)
+ .description("Network address of AMQP broker (e.g., localhost). If Brokers is set, then this property is ignored.")
+ .required(false)
.defaultValue("localhost")
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .addValidator(StandardValidators.NON_EMPTY_EL_VALIDATOR)
+ .addValidator(StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR)
Review comment:
I wanted to allow for this value to be empty, since BROKERS can now be supplied instead of HOST. But after some testing I've noticed that by providing a default value for HOST then it can never be empty, the default value will override empty values. So I can revert this change.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nabmoh123 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
nabmoh123 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-814967533
> I still think that a custom validate method makes sense. Especially to make the processor invalid if brokers, host, and port are all set. Because in that case it could confuse the users wrt to which properties are actually used. I acknowledge this is explained in the properties description but this would make things more "obvious".
>
> We do that for some processors where it's possible to configure the same "thing" via multiple properties after we added some new properties over time and we kept the old ones to not break backward compatibility.
>
> That's my only remaining comment, latest changes look good to me.
I could implement this, but it would require me to take off the default values for Host Name and Port. Otherwise, whenever Brokers is set, Host Name will take on its default value and cause a warning to appear. I'm not sure which is the lesser of two evils here, removing old functionality or not adding in clear and concise validation to ensure users understand which property is used.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-815002530
Yeah fair enough. Not in a position to merge right now but will do later today if no one else does before me.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4939:
URL: https://github.com/apache/nifi/pull/4939
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-816772902
Merged, thanks @nabmoh123
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nabmoh123 commented on a change in pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors
Posted by GitBox <gi...@apache.org>.
nabmoh123 commented on a change in pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#discussion_r607723417
##########
File path: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java
##########
@@ -317,7 +330,16 @@ public void handleUnexpectedConnectionDriverException(Connection conn, Throwable
});
try {
- Connection connection = cf.newConnection(executor);
+ Connection connection;
+ if (context.getProperty(BROKERS).isSet()) {
+ Address[] hostsList = createHostsList(context);
+ connection = cf.newConnection(executor, hostsList);
+ } else {
+ cf.setHost(context.getProperty(BROKERS).evaluateAttributeExpressions().getValue());
Review comment:
good spot, yes I'll correct this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org