You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@nifi.apache.org by ijokarumawak <gi...@git.apache.org> on 2016/11/11 12:31:05 UTC

[GitHub] nifi pull request #1208: NIFI-3026: Support multiple remote target URLs

GitHub user ijokarumawak opened a pull request:

    https://github.com/apache/nifi/pull/1208

    NIFI-3026: Support multiple remote target URLs

    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:
    - [x] Is there a JIRA ticket associated with this PR? Is it referenced 
         in the commit message?
    
    - [x] 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.
    
    - [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
    
    - [x] Is your initial contribution a single, squashed commit?
    
    ### For code changes:
    - [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
    - [x] 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:
    - [x] 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/ijokarumawak/nifi nifi-3026

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/nifi/pull/1208.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 #1208
    
----
commit 8f09b9ca1db1e4ac2d722645304b5b4a682ccaa5
Author: Koji Kawamura <ij...@apache.org>
Date:   2016-11-11T12:25:13Z

    NIFI-3026: Support multiple remote target URLs

----


---
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 #1208: NIFI-3026: Support multiple remote target URLs

Posted by markap14 <gi...@git.apache.org>.

Github user markap14 commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/1208#discussion_r87630105
  
    --- Diff: nifi-commons/nifi-site-to-site-client/src/main/java/org/apache/nifi/remote/client/SiteToSiteClient.java ---
    @@ -560,7 +560,8 @@ public SiteToSiteClient build() {
             }
     
             /**
    -         * @return the configured URL for the remote NiFi instance
    +         * @return the configured URL for the remote NiFi instance.
    +         * The URL string possibly contains multiple URL entries in comma-separated format.
              */
             public String getUrl() {
    --- End diff --
    
    This is concerning to me. This may be a breaking change for some, as it previously returned a String that was a valid URI or URL. It may not be anymore. I would rather see this return only the first URL, and have an additional method added: `List<String> getUrls();`
    
    Thoughts?


---
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 #1208: NIFI-3026: Support multiple remote target URLs

Posted by markap14 <gi...@git.apache.org>.

Github user markap14 commented on the issue:

https://github.com/apache/nifi/pull/1208

@ijokarumawak it was not the comment that concerns me. What concerns me is that we have a method named getUrl() (present in both SiteToSiteClientConfig and the SiteToSiteClient.Builder), and that method returns a String that is no longer a valid URL. Given that the method name is getUrl(), it is very reasonable to expect someone to write code like the following:

`final URL url = new URL(clientConfig.getUrl());`

And in fact we did something very similar in the client. This will now throw an Exception because getUrl() may not be returning a valid URL. There are many reasons that may be done - not just by someone implementing a client. Also, we can't assume that "the URL is what they configured by themselves." Certainly, code can be written to pass this object around. There's no telling where it came from.

We also must keep in mind that while Apache Flink, Storm and Spark integration may be safe, we have no idea what other projects may be using the client.

So my recommendation would be this: have a method named getUrls() that returns a comma-separated list of URL's, just like getUrl() does now in this PR. Then, add a getUrl() to maintain backward compatibility but make that method return the first URL available and mark it as deprecated with documentation as to why it's deprecated and why we now recommend use of getUrls() instead.

Does that seem reasonable?

---
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 #1208: NIFI-3026: Support multiple remote target URLs

Posted by ijokarumawak <gi...@git.apache.org>.

Github user ijokarumawak commented on the issue:

https://github.com/apache/nifi/pull/1208

Hi @markap14 @mcgilman

I've updated the PR commit with the latest master.
Explicitly separated the URI and URIs.

I've also updated these RPG UI components to use the plural URLs.
These display Name and URIs. It looked too verbose if URLs are used for both Name and URIs, so I used URI for the Name if Name is not specified:

Tried to update all UI components that I was aware of, but I didn't want to make this PR huge and rush to change following components, so that we can discuss more about the details with separated JIRAs.

## Known UI components those are not updated to use URLs

### Search component

RPGs can be searched by target URI. I haven't updated search related components to use plural URIs. So only the first URI is searchable, and shown in the UI right now.

### Component Status and Summary

### Action History

When RPG is updated, audit event is stored. I think we should use URIs here. But in order to do so, we have to alter the REMOTE_PROCESS_GROUP_DETAILS table in database repository. I don't know what would be a proper migration process at this point, so didn't touch it within this PR.

Hope this PR would be sufficient to provide the core support of multiple URIs for better connectivity with NiFi cluster via Site-to-Site. Thanks for reviewing!

[GitHub] nifi issue #1208: NIFI-3026: Support multiple remote target URLs

Posted by ijokarumawak <gi...@git.apache.org>.

Github user ijokarumawak commented on the issue:

https://github.com/apache/nifi/pull/1208

@markap14 I checked how Apache Flink uses SiteToSiteClient from [NiFiSink](https://github.com/apache/flink/blob/d7b59d761601baba6765bb4fc407bcd9fd6a9387/flink-streaming-connectors/flink-connector-nifi/src/test/java/org/apache/flink/streaming/connectors/nifi/examples/NiFiSinkTopologyExample.java). That is the same pattern Storm and Spark integration does.
I agree that third parties have been using SiteToSiteClient, but I don't think many of them re-implement SiteToSiteClient actual implementation, rather they just use the library. When they use it just to use SiteToSite protocol, the URL is what they configured by themselves. So it should be fine. It doesn't break anything.

For the case if a third party actually re-implementing a SiteToSiteClient to customize it at lower level, excuse me, the comment you concerned about was misleading. The interface hasn't been changed at all. What has changed by this PR is the behavior of one implementation of SiteToSiteClient, that is the NiFi default implementation, i.e. SocketClient and HttpClient. I didn't have to change the comment in the first place. It doesn't break any other SiteToSiteClient implementations written by third parties outside of NiFi codebase.

So, to address your concern, I just removed the comment from SiteToSiteClient.java. By doing so, the file becomes identical with the previous version. So I amended the commit to keep it intact.