You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by markap14 <gi...@git.apache.org> on 2016/07/20 16:22:12 UTC
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
GitHub user markap14 opened a pull request:
https://github.com/apache/nifi/pull/688
NIFI-2304: If node address is not set, default to localhost, as the a\u2026
\u2026dmin guide says it should
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/markap14/nifi NIFI-2304
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/688.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #688
----
commit c7415177426d7a18a7bdb54af0b57066dc5d5dbb
Author: Mark Payne <ma...@hotmail.com>
Date: 2016-07-20T16:21:58Z
NIFI-2304: If node address is not set, default to localhost, as the admin guide says it should
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72614705
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
--- End diff --
@ijokarumawak I just updated the PR to check if empty as well, since I do think the error message is cleaner in that case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72561906
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
--- End diff --
There's another similar null check against port `if (port == null) {`. I think it should be addressed, too.
We may be able to take advantage of our existing StringUtils.isBlank().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72561569
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
+ hostname = "localhost";
--- End diff --
Is there any specific reason to change from `InetAddress.getLocalHost().getHostName()` to `"localhost"`?
I thought if server's hostname is properly configured at OS level, administrator doesn't have to set hostname in nifi.properties, which makes configuration management easier since they can use the same properties file among nodes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #688: NIFI-2304: If node address is not set, default to localhost...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/688
@markap14 I think the above exception message I posted earlier is a separate issue. Please file another JIRA issue if you feel the log message should be more user friendly.
I am going to merge this since this PR solves the original issue, that is use localhost if cluster node address is blank in nifi.properties.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72614233
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
--- End diff --
For port, we do validate that the port can be parsed via Integer.parseInt -- if empty string, that will fail. I guess the error message may be more clear if we explicitly check for empty string though and indicate that the property is not specified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/688
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #688: NIFI-2304: If node address is not set, default to localhost...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/688
@markap14 Thanks for fixing this. I've encountered the same issue as reported in NIFI-2304, too.
Started reviewing, and added few comments. Please let me know how you think. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #688: NIFI-2304: If node address is not set, default to localhost...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/688
@markap14 Thanks for the updates!
I tested the latest PR, set `nifi.cluster.node.address` as blank in nifi.properties, confirmed that e node NiFi cluster can start up successfully.
However, for `nifi.cluster.node.protocol.port`, there's another usage of that configuration, and I got following error message with blank property value:
```
Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'protocolListener' defined in class path resource [nifi-cluster-protocol-context.xml]: Unsatisfied dependency expressed through constructor argument with index 1 of type [int]: Could not convert constructor argument value of type [null] to required type [int]: Failed to convert value of type [null] to required type [int]; nested exception is java.lang.IllegalArgumentException: Cannot convert value of type [null] to required type [int]: PropertyEditor [org.springframework.beans.propertyeditors.CustomNumberEditor] returned inappropriate value of type [null]
at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:724) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:185) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.autowireConstructor(AbstractAutowireCapableBeanFactory.java:1143) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1046) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:510) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:482) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractBeanFactory$1.getObject(AbstractBeanFactory.java:306) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:230) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:302) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:197) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
at org.springframework.beans.factory.support.BeanDefinitionValueResolver.resolveReference(BeanDefinitionValueResolver.java:351) ~[spring-beans-4.2.4.RELEASE.jar:4.2.4.RELEASE]
... 52 common frames omitted
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72731867
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
+ hostname = "localhost";
--- End diff --
Thanks for the explanation. I found that [docs says so](https://nifi.apache.org/docs/nifi-docs/html/administration-guide.html#clustering).
I have read `nifi.remote.input.socket.host` description before, and wondered what's the reason of these difference:
```
By default, it is the value from InetAddress.getLocalHost().getHostName(). On UNIX-like operating systems, this is typically the output from the hostname command.
```
I understand that as to keep how it was done in 0.x.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #688: NIFI-2304: If node address is not set, default to lo...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/688#discussion_r72613879
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/heartbeat/ClusterProtocolHeartbeatMonitor.java ---
@@ -91,12 +91,8 @@ public ClusterProtocolHeartbeatMonitor(final ClusterCoordinator clusterCoordinat
this.clusterNodesPath = zkClientConfig.resolvePath("cluster/nodes");
String hostname = properties.getProperty(NiFiProperties.CLUSTER_NODE_ADDRESS);
- if (hostname == null) {
- try {
- hostname = InetAddress.getLocalHost().getHostName();
- } catch (UnknownHostException e) {
- throw new RuntimeException("Unable to determine local hostname and the '" + NiFiProperties.CLUSTER_NODE_ADDRESS + "' property is not set");
- }
+ if (hostname == null || hostname.trim().isEmpty()) {
+ hostname = "localhost";
--- End diff --
@ijokarumawak the reason that I explicitly changed it to 'localhost' is because this is what the documentation says will be used. This is how it was done in 0.x, as well. I think the reason for this is that we want to urge admins to configure it, because getLocalHost().getHostName() may not necessarily return the correct result if there are multiple network interfaces, which is quite common in production systems. For example, the hostname on a private network may be nifi-01 while the hostname on the public lan will be r00s80-hq.mycompany.com. So getLocalHost() i believe will return whatever the 'hostname' command returns (in linux at least). But there's a good chance that we want to use the private LAN for cluster comms. So this just sort of urges the admin to configure it... at least, I think that's why the decision was made to use "localhost." Not sure since I didn't make the decision :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #688: NIFI-2304: If node address is not set, default to localhost...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/688
@ijokarumawak thanks for reviewing! Updated PR to address the 'port' and left comments re: hostname. Let me know if you have any other thoughts. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---