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 2017/03/01 20:16:00 UTC
[GitHub] nifi pull request #1550: Nifi 3541
GitHub user markap14 opened a pull request:
https://github.com/apache/nifi/pull/1550
Nifi 3541
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] 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)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/markap14/nifi NIFI-3541
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1550.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 #1550
----
commit 09c5b22c6aeb726f6d8cbb8945c4e1126bc27b68
Author: Mark Payne <ma...@hotmail.com>
Date: 2017-03-01T18:30:04Z
NIFI-3541: Add local network interface capability to site-to-site client and remote group and ports
commit eb2ccd9f3925d04bec1198df230f2f0e7d20d97e
Author: Matt Gilman <ma...@gmail.com>
Date: 2017-03-01T18:51:34Z
NIFI-3541: - Allowing the user to specify the network interface to send/receive data for a Remote Process Group.
Signed-off-by: Mark Payne <ma...@hotmail.com>
commit 7b797b4426a34129ef493be91e81f6f8af72b2aa
Author: Mark Payne <ma...@hotmail.com>
Date: 2017-03-01T20:15:31Z
NIFI-3541: Updated serializer / synchronizer / fingerprintfactory to account for local network interface
----
---
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 #1550: Nifi 3541
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi/pull/1550
@markap14 code looks good and I was able to test the anticipated functionality. I think we likely need to get some doc additions to the System Admin guide that covers this functionality, how to use it, and the caveats (it is my understanding that not all systems will adhere to the suggested guidance on what interface to use).
---
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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@phrocker my testing consisted basically of setting invalid values, verifying that the RPG became invalid, and then setting it to a valid value and ensuring that things then worked... if you have ideas about how to test more rigorously that would be good.
---
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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@apiri hmmm.... that's odd... perhaps I missed a file in the 'git add' before i did the 'git commit'?? Will look into it.
---
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 #1550: Nifi 3541
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1550
---
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 #1550: Nifi 3541
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1550#discussion_r103794598
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/remote/StandardRemoteProcessGroup.java ---
@@ -856,10 +864,75 @@ public void refreshFlowContents() throws CommunicationsException {
}
}
+ @Override
+ public String getNetworkInterface() {
+ readLock.lock();
+ try {
+ return networkInterfaceName;
+ } finally {
+ readLock.unlock();
+ }
+ }
+
+ @Override
+ public void setNetworkInterface(final String interfaceName) {
+ writeLock.lock();
+ try {
+ this.networkInterfaceName = interfaceName;
+
+ try {
+ final Enumeration<InetAddress> inetAddresses = NetworkInterface.getByName(interfaceName).getInetAddresses();
+
+ if (inetAddresses.hasMoreElements()) {
+ this.localAddress = inetAddresses.nextElement();
--- End diff --
@markap14 on an unrelated note and this while I approved it I didn't see a call to use any address when local address isn't specified. I imagine that's related to @apiri's comment, below, but I haven't inquired about his issue.
---
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 #1550: Nifi 3541
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi/pull/1550
Started looking at this a bit and still testing, but it appears that if an interface is not specified, it causes a validation error.
---
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 #1550: Nifi 3541
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi/pull/1550
Approved on static review only since @apiri is running a test near me.
---
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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@apiri I have updated the commit. Please give it a try again. 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 pull request #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1550#discussion_r103784862
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/remote/StandardRemoteProcessGroup.java ---
@@ -856,10 +864,75 @@ public void refreshFlowContents() throws CommunicationsException {
}
}
+ @Override
+ public String getNetworkInterface() {
+ readLock.lock();
+ try {
+ return networkInterfaceName;
+ } finally {
+ readLock.unlock();
+ }
+ }
+
+ @Override
+ public void setNetworkInterface(final String interfaceName) {
+ writeLock.lock();
+ try {
+ this.networkInterfaceName = interfaceName;
+
+ try {
+ final Enumeration<InetAddress> inetAddresses = NetworkInterface.getByName(interfaceName).getInetAddresses();
+
+ if (inetAddresses.hasMoreElements()) {
+ this.localAddress = inetAddresses.nextElement();
--- End diff --
@phrocker you are right - the user is selecting a name that could potentially map to multiple IP's. I'm not sure what the right approach to this is. This seemed like a reasonable option to me. The only other option that I could think of would be to require that both a network interface and a hostname/IP be provided. But for a clustered environment, that won't work. So picking the first one seemed the best option to me. Any other suggestion?
---
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 #1550: Nifi 3541
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1550#discussion_r103780999
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/remote/StandardRemoteProcessGroup.java ---
@@ -856,10 +864,75 @@ public void refreshFlowContents() throws CommunicationsException {
}
}
+ @Override
+ public String getNetworkInterface() {
+ readLock.lock();
+ try {
+ return networkInterfaceName;
+ } finally {
+ readLock.unlock();
+ }
+ }
+
+ @Override
+ public void setNetworkInterface(final String interfaceName) {
+ writeLock.lock();
+ try {
+ this.networkInterfaceName = interfaceName;
+
+ try {
+ final Enumeration<InetAddress> inetAddresses = NetworkInterface.getByName(interfaceName).getInetAddresses();
+
+ if (inetAddresses.hasMoreElements()) {
+ this.localAddress = inetAddresses.nextElement();
--- End diff --
Is there any reason this could provide us an unintended result?
---
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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@apiri that's a good call. I just pushed a new commit that updates the User Guide, where these properties are explained. 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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@apiri good catch. Will address.
---
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 #1550: Nifi 3541
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1550#discussion_r103793832
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/remote/StandardRemoteProcessGroup.java ---
@@ -856,10 +864,75 @@ public void refreshFlowContents() throws CommunicationsException {
}
}
+ @Override
+ public String getNetworkInterface() {
+ readLock.lock();
+ try {
+ return networkInterfaceName;
+ } finally {
+ readLock.unlock();
+ }
+ }
+
+ @Override
+ public void setNetworkInterface(final String interfaceName) {
+ writeLock.lock();
+ try {
+ this.networkInterfaceName = interfaceName;
+
+ try {
+ final Enumeration<InetAddress> inetAddresses = NetworkInterface.getByName(interfaceName).getInetAddresses();
+
+ if (inetAddresses.hasMoreElements()) {
+ this.localAddress = inetAddresses.nextElement();
--- End diff --
@markap14 One option might be to return all InetAddresses within the enumeration but this doesn't seem necessary for an unlikely case. I'm not sure that approach is even necessary or makes sense. Given that you clearly evaluated that and other options I'm good. The assumption is clear to me.
---
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 #1550: Nifi 3541
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/1550
@apiri I have pushed a new commit that adds in just that file. 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 pull request #1550: Nifi 3541
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1550#discussion_r103783512
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/remote/StandardRemoteProcessGroup.java ---
@@ -856,10 +864,75 @@ public void refreshFlowContents() throws CommunicationsException {
}
}
+ @Override
+ public String getNetworkInterface() {
+ readLock.lock();
+ try {
+ return networkInterfaceName;
+ } finally {
+ readLock.unlock();
+ }
+ }
+
+ @Override
+ public void setNetworkInterface(final String interfaceName) {
+ writeLock.lock();
+ try {
+ this.networkInterfaceName = interfaceName;
+
+ try {
+ final Enumeration<InetAddress> inetAddresses = NetworkInterface.getByName(interfaceName).getInetAddresses();
+
+ if (inetAddresses.hasMoreElements()) {
+ this.localAddress = inetAddresses.nextElement();
--- End diff --
I guess my thought is that the user is selecting a name that results in a mapping. What if one of the objects within the enumeration is at some point invalid, doesn't that break the user's assumption that he or she would always get a valid InetAddress that is bound to that network interface name?
---
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 #1550: Nifi 3541
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi/pull/1550
Hey @markap14,
Looks like we are getting a compilation error in SSLCommsSession.
---
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.
---