You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by christophercurrie <gi...@git.apache.org> on 2017/09/10 21:22:34 UTC
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
GitHub user christophercurrie opened a pull request:
https://github.com/apache/nifi/pull/2140
NIFI-3950 Refactor AWS bundle
This PR provides a separate NAR for the `AWSCredentialsProviderService` API, and a new `nifi-aws-abstract-processors` jar so that custom processors can take advantage of the abstract base classes without importing duplicates of the existing implementations.
The JIRA ticket mentions the need for a transition plan; I'm not sure what is required there, but happy to help provide whatever is needed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/christophercurrie/nifi NIFI-3950-aws-refactor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2140.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 #2140
----
commit 1c30e78716dd9f592cdbe9abc583053cb7fcd31d
Author: Christopher Currie <ch...@currie.com>
Date: 2017-09-10T20:19:45Z
NIFI-3950 Refactor AWS bundle
----
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
Please add the commits to this one. It usually helps reviewing to see the commits separately, and it's easy enough to squash at the end.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138257624
--- Diff: pom.xml ---
@@ -1109,12 +1109,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-kudu-nar</artifactId>
--- End diff --
It was duplicated in the pom; Maven issued a warning to me about it.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138257661
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/pom.xml ---
@@ -253,11 +253,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-schema-registry-service-api</artifactId>
--- End diff --
It was duplicated in the pom, once as a test dep and once as a regular dep. Maven warned me about it.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
Yes, though I'm not sure what action items are left for me at this point.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138704486
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-nar/pom.xml ---
@@ -32,13 +32,14 @@
<dependencies>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-standard-services-api-nar</artifactId>
+ <artifactId>nifi-aws-service-api-nar</artifactId>
+ <version>${project.version}</version>
<type>nar</type>
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-aws-processors</artifactId>
- <version>1.4.0-SNAPSHOT</version>
--- End diff --
We actually had a problem in the past with the release plugin and project.version, I can't remember the specifics and it may very well not be an issue anymore, but we did specifically make the choice to use the literal version everywhere so I'd prefer to stick with that
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138248781
--- Diff: nifi-assembly/pom.xml ---
@@ -1,13 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!-- 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
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
--- End diff --
Several of the diffs in this PR are whitespace-only changes. Are these intentional?
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/2140
Regarding the migration strategy... Lets make sure we document on the migration wiki page [1] that anyone who built custom components using the AWS controller service should rebuild them with the new dependency as part of their upgrade process.
Given our component versioning, it would actually be possible to run the old stuff and the new stuff at the same time, but it would require a complicated setup. I think you'd have to leave the 1.3.0 AWS NAR, 1.3.0 standard services API NAR, and possibly 1.3.0 standard NAR, plus the users custom NAR that depended on 1.3.0 AWS NAR. Probably not the recommended approach, but a fallback option.
[1] https://cwiki.apache.org/confluence/display/NIFI/Migration+Guidance
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138251749
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-nar/pom.xml ---
@@ -32,13 +32,14 @@
<dependencies>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-standard-services-api-nar</artifactId>
+ <artifactId>nifi-aws-service-api-nar</artifactId>
+ <version>${project.version}</version>
<type>nar</type>
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-aws-processors</artifactId>
- <version>1.4.0-SNAPSHOT</version>
--- End diff --
I'm not well informed on the pros and cons of using the project.version variable as opposed to the literal version number. The NiFi project as a whole manages literal version numbers with the Maven Release Plugin , so the variables are not necessary to keep references consistent. What has been your experience?
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138521026
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/pom.xml ---
@@ -253,11 +253,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-schema-registry-service-api</artifactId>
--- End diff --
Again, a great change in a different PR for a different ticket.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138257973
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-nar/pom.xml ---
@@ -32,13 +32,14 @@
<dependencies>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-standard-services-api-nar</artifactId>
+ <artifactId>nifi-aws-service-api-nar</artifactId>
+ <version>${project.version}</version>
<type>nar</type>
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-aws-processors</artifactId>
- <version>1.4.0-SNAPSHOT</version>
--- End diff --
It's a style choice, one I follow out of habit. The release plugin is perfectly happy with such references, and I find it clearer to declare the version once at the the parent pom reference and stick with it, unless the version is deliberately varying. But that's for your project to decide, so happy to revert my habit here if wanted.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
The failed CI builds seem to be failing for other PRs and do not appear to be caused by this change.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
@christophercurrie, thanks again for the work on this infrastructure project.
I merged these commits to master, except for the last commit "Limit NOTICE to AWS SDK" for nifi-aws-service-api-nar. I lied about not needed all those dependency notices. After checking with Maven, I understand HttpComponents, Joda Time, etc. are transitive dependencies of the AWS SDK, and we need them even for the interfaces. I apologize for the confusion about that, and thank you for adding it as a separate commit.
I'll follow up with some text for the NiFi 1.5.0 migration notes.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
@christophercurrie The outstanding item for now is a set of LICENSE/NOTICE files for nifi-aws-service-api-nar, similar to what is now in the nifi-aws-nar. I believe the NOTICE file can be pared down to only reference the aws-sdk.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138251181
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/pom.xml ---
@@ -253,11 +253,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-schema-registry-service-api</artifactId>
--- End diff --
Is nifi-schema-registry-service-api intentionally removed here?
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2140
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138520916
--- Diff: pom.xml ---
@@ -1109,12 +1109,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-kudu-nar</artifactId>
--- End diff --
I understand. I think that might make a great change in a different PR for a different ticket. But again, I would prefer not to evaluate it as part of this one.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
Ok, I've added those files. Let me know if they look OK and if there's anything else that needs doing.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
Reviewing... I agree the CI failure may not be related to this change. `mvn clean install -Pcontrib-check` worked OK for me.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138257582
--- Diff: nifi-assembly/pom.xml ---
@@ -1,13 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!-- 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
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
--- End diff --
Not deliberate, no, probably a result of my editor settings; I can revert them if they are unwanted.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
@jvwing I have pushed a commit that removes all unnecessary changes from the PR. Sorry for the delay.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
Not a problem on the reverts. The original instructions asked for a squashed commit. Should I create a new smaller PR, or add the revert commit to this one?
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
@christophercurrie I think we're pretty close on this PR, any interest in continuing?
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
@christophercurrie - With respect to a transition plan, I'm not sure exactly what we need. I'll have to get back to you on that. In vague concept, users who have built custom processors and custom controller services against the existing API should have a smooth upgrade experience to the new one. I'll try to work out a more concrete definition for 'smooth'.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/2140
@jvwing sorry I never responded to this... The results you got seem correct.
The part I wasn't sure about was that the `nifi-aws-nar-1.3.0.nar` has a dependency on` nifi-standard-services-api-nar-1.3.0.nar`, so I thought you may have to leave the whole change of dependencies around.
Looking at the code during start up, I think when loading the `nifi-aws-nar-1.3.0.nar` we would detect that there is no 1.3.0 version of `standard-services-api-nar` present, but there is a 1.4.0 version so just use that, which is probably why it worked.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
Thanks, @bbende, I'll take up the migration guidance for the wiki. As part of reviewing this PR, I am building some sample processor and service projects to reproduce the problems and check the fix. I plan to work through the migration steps myself and can document the process.
It's very likely that I'll have more questions for you as I get into details of it.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by christophercurrie <gi...@git.apache.org>.
Github user christophercurrie commented on the issue:
https://github.com/apache/nifi/pull/2140
How smooth their experience will be will probably depend upon their internal project practices. Since the `nifi-aws-processors` package would have been a required dependency for such custom processors, the main change would be to require adding `nifi-aws-service-api-nar` somewhere in their nar parent chain; this change is, AFAICT, unavoidable given the nature of controller-processor decoupling.
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138251087
--- Diff: pom.xml ---
@@ -1109,12 +1109,6 @@
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
- <artifactId>nifi-kudu-nar</artifactId>
--- End diff --
Did you intend to remove nifi-kudu-nar?
---
[GitHub] nifi pull request #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2140#discussion_r138520837
--- Diff: nifi-assembly/pom.xml ---
@@ -1,13 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!-- 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
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
--- End diff --
I would appreciate it if you reverted them. It is easier to review a PR if the changes are focused narrowly on the subject of the ticket.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
Thanks for the update, @christophercurrie . This PR is looking pretty good:
* Passes the full suite of unit tests with contrib-check.
* AWS processors and controller service still work OK in my testing.
* Provides a good migration experience -- just rebuild against NiFi 1.4.0 nars -- better than I feared. More below.
One thing we still need is a set of LICENSE/NOTICE files for nifi-aws-service-api-nar, similar to what is now in the nifi-aws-nar. I believe the NOTICE file can be pared down to only reference the aws-sdk.
**Migration Experience**
I created a [simple AWS bundle](https://github.com/jvwing/sample-aws-bundle) targeting NiFi 1.3.0, and went through the exercise of [migrating it](https://github.com/jvwing/sample-aws-bundle/tree/target-nifi-1.4.0) to 1.4.0 as of this PR. It seems "smooth" enough to me.
* Advancing the NiFi dependency version to 1.4.0 and rebuilding is enough, maintaining the NAR dependency on `nifi-aws-nar`.
* For bundles that only implement controller service interfaces, they may optionally change their NAR dependency to `nifi-aws-service-api-nar`. Since nifi-aws-nar already has this NAR dependency, I believe this is a recommended, but not strictly necessary step.
---
[GitHub] nifi issue #2140: NIFI-3950 Refactor AWS bundle
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2140
More migration notes:
* My 1.3.0 AWS processor worked OK without modification when used with this PR.
* My 1.3.0 AWS controller service did not work with just the NAR file, with the expected incompatibility error:
> AWS Credentials Provider service' validated against '8902b809-015e-1000-46c3-e321c1d9a1b4' is invalid because SampleAWSCreds - 1.3.0 from sample - sample-aws-services-nar is not compatible with AWSCredentialsProviderService - 1.4.0-SNAPSHOT from org.apache.nifi - nifi-aws-nar
* However, if `nifi-aws-nar-1.3.0.nar` was included side-by-side with `nifi-aws-nar-1.4.0.nar`, the 1.3.0 processor worked with the 1.3.0 controller service, unchanged.
@bbende , I'm not sure the last option is what you described above. I was expecting to add more NARs. But it seemed plausible that the monolithic nature of nifi-aws-nar-1.3.0.nar might make it easier to dump in side-by-side?
---