You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by snagacarl <gi...@git.apache.org> on 2018/10/05 17:36:14 UTC
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
GitHub user snagacarl opened a pull request:
https://github.com/apache/nifi/pull/3049
NIFI-5664 Support ArrayList in DataTypeUtils#toArray
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:
- [Y] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [Y] 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.
- [Y] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [Y] Is your initial contribution a single, squashed commit?
### For code changes:
- [Y] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [Y] Have you written or updated unit tests to verify your changes?
- [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?
- [N/A] 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 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/Snagajob/nifi NIFI-5664
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3049.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 #3049
----
commit 30d0410f8f35cff772cc74fc36e4368d33310aec
Author: Carl Gieringer <ca...@...>
Date: 2018-10-04T16:50:08Z
NIFI-5664 Support ArrayList in DataTypeUtils#toArray
----
---
[GitHub] nifi issue #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArray
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on the issue:
https://github.com/apache/nifi/pull/3049
Great suggestions. I can incorporate and re-test this week. Thanks for your feedback!
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223181320
--- Diff: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java ---
@@ -339,6 +339,11 @@ public static boolean isRecordTypeCompatible(final Object value) {
return dest;
}
+ if (value instanceof ArrayList) {
--- End diff --
I agree with @ottobackwards. It really should not be a specific implementation.
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/nifi/pull/3049
This looks great to me, I thought there was a specific use case for this, if so, it might be good to have a unit or integration text that showed that case.
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223396416
--- Diff: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java ---
@@ -168,6 +168,21 @@ public void testConvertRecordFieldToObject() {
}
+ @Test
+ public void testToArray() {
+ final ArrayList<String> arrayList = new ArrayList<>();
+ arrayList.add("Seven");
--- End diff --
I have updated the PR. Thanks!
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on the issue:
https://github.com/apache/nifi/pull/3049
Now that the checks have passed, I believe this PR is G2G as far as comments so far are concerned.
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223396314
--- Diff: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java ---
@@ -168,6 +168,21 @@ public void testConvertRecordFieldToObject() {
}
+ @Test
+ public void testToArray() {
+ final ArrayList<String> arrayList = new ArrayList<>();
--- End diff --
Good catch. Updated in the PR now.
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/3049
@snagacarl since this was for the Mongo lookup service, wanted to make sure that you're aware that in 1.8 there will be a slight breaking change. The "configuration" controller service and the lookup service are now cleanly separated so that the lookup service can be cheerfully ignorant of connection details like the URL, SSL and authentication details. That way plugging in a new cluster or something will be faster and easier for Mongo users.
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223181352
--- Diff: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java ---
@@ -168,6 +168,21 @@ public void testConvertRecordFieldToObject() {
}
+ @Test
+ public void testToArray() {
+ final ArrayList<String> arrayList = new ArrayList<>();
+ arrayList.add("Seven");
--- End diff --
This could also be reduced to `final list<String> arrayList = Arrays.asList("Seven", "Eleven", "Thirteen");`
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on the issue:
https://github.com/apache/nifi/pull/3049
I rebased onto master, FYI.
---
[GitHub] nifi issue #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArray
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on the issue:
https://github.com/apache/nifi/pull/3049
Apache issue: https://issues.apache.org/jira/browse/NIFI-5664
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on the issue:
https://github.com/apache/nifi/pull/3049
Yes, I did test the changes in 1.8 and I noticed that change to how the MongoDBLookupService uses a MongoDBControllerService. Thanks for the heads up!
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223141579
--- Diff: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java ---
@@ -339,6 +339,11 @@ public static boolean isRecordTypeCompatible(final Object value) {
return dest;
}
--- End diff --
Why wouldn't this be any List implementation?
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/3049
Cool. FYI, the long term goal is to make controller services the exclusive source of configurations for configurations. That's because a single `MongoClient` is really a connection pool and is--according to Mongo Inc.--thread-safe and intended to be used across an entire app. That should help Mongo users tighten up on how many connections they have open from NiFi, especially a cluster (since each node has N number of MongoClients where N = # of mongo processors).
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223393416
--- Diff: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java ---
@@ -339,6 +339,11 @@ public static boolean isRecordTypeCompatible(final Object value) {
return dest;
}
--- End diff --
I have updated the PR. Thanks for the good idea!
---
[GitHub] nifi pull request #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/3049
---
[GitHub] nifi issue #3049: NIFI-5664 Support List in DataTypeUtils#toArray
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/3049
I'll try to review tonight. Been pretty sick. Thanks for the updates @snagacarl
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by snagacarl <gi...@git.apache.org>.
Github user snagacarl commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223396193
--- Diff: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java ---
@@ -339,6 +339,11 @@ public static boolean isRecordTypeCompatible(final Object value) {
return dest;
}
+ if (value instanceof ArrayList) {
--- End diff --
Great idea. I have updated.
---
[GitHub] nifi pull request #3049: NIFI-5664 Support ArrayList in DataTypeUtils#toArra...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3049#discussion_r223181332
--- Diff: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java ---
@@ -168,6 +168,21 @@ public void testConvertRecordFieldToObject() {
}
+ @Test
+ public void testToArray() {
+ final ArrayList<String> arrayList = new ArrayList<>();
--- End diff --
Should be `final List<String>....`
---