You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/04/06 14:51:59 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #5900: NIFI-7234 Standardized on Avro 1.11.0
exceptionfactory commented on code in PR #5900:
URL: https://github.com/apache/nifi/pull/5900#discussion_r844029990
##########
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/pom.xml:
##########
@@ -57,6 +56,11 @@
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-record</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-databind</artifactId>
+ <version>2.13.1</version>
Review Comment:
This specific version can be removed so that it leverages the version that `jackson-bom` provides.
```suggestion
<version>2.13.1</version>
```
```suggestion
```
##########
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/pom.xml:
##########
@@ -57,6 +56,11 @@
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-record</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-databind</artifactId>
Review Comment:
Is there a reason for introducing the Jackson dependency in this module as part of this change?
##########
nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/pom.xml:
##########
@@ -140,6 +140,11 @@
<artifactId>nifi-record</artifactId>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-annotations</artifactId>
+ <version>2.13.1</version>
Review Comment:
The version should be removed to leverage the value from `jackson-bom`.
```suggestion
<version>2.13.1</version>
```
```suggestion
```
##########
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml:
##########
@@ -68,6 +68,11 @@
<artifactId>nifi-kerberos-credentials-service-api</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.avro</groupId>
+ <artifactId>avro</artifactId>
+<!-- <version>1.8.1</version>-->
Review Comment:
The version should be removed instead of commented out.
```suggestion
<!-- <version>1.8.1</version>-->
```
```suggestion
```
##########
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml:
##########
@@ -68,6 +68,11 @@
<artifactId>nifi-kerberos-credentials-service-api</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.avro</groupId>
+ <artifactId>avro</artifactId>
+<!-- <version>1.8.1</version>-->
Review Comment:
The version should be removed instead of commented out.
```suggestion
<!-- <version>1.8.1</version>-->
```
```suggestion
```
##########
nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/AbstractSiteToSiteReportingTask.java:
##########
@@ -47,13 +54,6 @@
import org.apache.nifi.serialization.record.type.MapDataType;
import org.apache.nifi.serialization.record.type.RecordDataType;
import org.apache.nifi.serialization.record.util.DataTypeUtils;
-import org.codehaus.jackson.JsonFactory;
-import org.codehaus.jackson.JsonNode;
-import org.codehaus.jackson.JsonParseException;
-import org.codehaus.jackson.JsonParser;
-import org.codehaus.jackson.JsonToken;
-import org.codehaus.jackson.map.ObjectMapper;
-import org.codehaus.jackson.node.ArrayNode;
Review Comment:
This is a useful improvement, but is it required as part of this upgrade? Just looking to confirm the impact scope of the Avro upgrade.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java:
##########
@@ -63,10 +67,6 @@
import org.apache.nifi.processor.io.InputStreamCallback;
import org.apache.nifi.processor.io.OutputStreamCallback;
import org.apache.nifi.processor.util.StandardValidators;
-import org.codehaus.jackson.JsonNode;
-import org.codehaus.jackson.map.ObjectMapper;
-import org.codehaus.jackson.node.ArrayNode;
-import org.codehaus.jackson.node.JsonNodeFactory;
Review Comment:
Is this change required as part of the upgrade?
##########
nifi-nar-bundles/nifi-sql-reporting-bundle/nifi-sql-reporting-tasks/pom.xml:
##########
@@ -79,6 +79,11 @@
<artifactId>caffeine</artifactId>
<version>2.8.1</version>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-core</artifactId>
+ <version>2.13.1</version>
Review Comment:
The version should be removed.
```suggestion
<version>2.13.1</version>
```
```suggestion
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/pom.xml:
##########
@@ -134,11 +133,26 @@
<artifactId>bval-jsr</artifactId>
<version>2.0.5</version>
</dependency>
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-mapper-asl</artifactId>
+ <version>1.9.13</version>
+ </dependency>
Review Comment:
Is this dependency still required with the refactored classes that use Jackson 2?
##########
nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/pom.xml:
##########
@@ -140,6 +140,11 @@
<artifactId>nifi-record</artifactId>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-annotations</artifactId>
+ <version>2.13.1</version>
Review Comment:
The version should be removed to leverage the value from `jackson-bom`.
```suggestion
<version>2.13.1</version>
```
```suggestion
```
##########
nifi-nar-bundles/nifi-hadoop-libraries-bundle/nifi-hadoop-libraries-nar/pom.xml:
##########
@@ -20,7 +20,7 @@
<properties>
<maven.javadoc.skip>true</maven.javadoc.skip>
<source.skip>true</source.skip>
- <avro.version>1.8.1</avro.version>
+<!-- <avro.version>1.8.1</avro.version>-->
Review Comment:
It looks like the version line should be removed instead of commented out.
```suggestion
<!-- <avro.version>1.8.1</avro.version>-->
```
```suggestion
```
##########
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml:
##########
@@ -214,5 +219,9 @@
<version>1.17.0-SNAPSHOT</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.xerial.snappy</groupId>
+ <artifactId>snappy-java</artifactId>
+ </dependency>
Review Comment:
Is it necessary to introduce the explicit dependency on `snappy-java`? Is it it no longer a transitive dependency of Avro?
##########
nifi-nar-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/AbstractSiteToSiteReportingTask.java:
##########
@@ -47,13 +54,6 @@
import org.apache.nifi.serialization.record.type.MapDataType;
import org.apache.nifi.serialization.record.type.RecordDataType;
import org.apache.nifi.serialization.record.util.DataTypeUtils;
-import org.codehaus.jackson.JsonFactory;
-import org.codehaus.jackson.JsonNode;
-import org.codehaus.jackson.JsonParseException;
-import org.codehaus.jackson.JsonParser;
-import org.codehaus.jackson.JsonToken;
-import org.codehaus.jackson.map.ObjectMapper;
-import org.codehaus.jackson.node.ArrayNode;
Review Comment:
This is a useful improvement, but is it required as part of this upgrade? Just looking to confirm the impact scope of the Avro upgrade.
##########
nifi-nar-bundles/nifi-sql-reporting-bundle/nifi-sql-reporting-tasks/pom.xml:
##########
@@ -79,6 +79,11 @@
<artifactId>caffeine</artifactId>
<version>2.8.1</version>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-core</artifactId>
+ <version>2.13.1</version>
Review Comment:
The version should be removed.
```suggestion
<version>2.13.1</version>
```
```suggestion
```
##########
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/pom.xml:
##########
@@ -214,5 +219,9 @@
<version>1.17.0-SNAPSHOT</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.xerial.snappy</groupId>
+ <artifactId>snappy-java</artifactId>
+ </dependency>
Review Comment:
Is it necessary to introduce the explicit dependency on `snappy-java`? Is it it no longer a transitive dependency of Avro?
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ConvertJSONToSQL.java:
##########
@@ -63,10 +67,6 @@
import org.apache.nifi.processor.io.InputStreamCallback;
import org.apache.nifi.processor.io.OutputStreamCallback;
import org.apache.nifi.processor.util.StandardValidators;
-import org.codehaus.jackson.JsonNode;
-import org.codehaus.jackson.map.ObjectMapper;
-import org.codehaus.jackson.node.ArrayNode;
-import org.codehaus.jackson.node.JsonNodeFactory;
Review Comment:
Is this change required as part of the upgrade?
##########
nifi-nar-bundles/nifi-hadoop-libraries-bundle/nifi-hadoop-libraries-nar/pom.xml:
##########
@@ -20,7 +20,7 @@
<properties>
<maven.javadoc.skip>true</maven.javadoc.skip>
<source.skip>true</source.skip>
- <avro.version>1.8.1</avro.version>
+<!-- <avro.version>1.8.1</avro.version>-->
Review Comment:
It looks like the version line should be removed instead of commented out.
```suggestion
<!-- <avro.version>1.8.1</avro.version>-->
```
```suggestion
```
##########
pom.xml:
##########
@@ -269,6 +270,17 @@
<artifactId>bcpg-jdk15on</artifactId>
<version>${org.bouncycastle.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.avro</groupId>
+ <artifactId>avro</artifactId>
+ <version>${avro.version}</version>
+<!-- <type>bundle</type>-->
Review Comment:
This comment should be removed.
```suggestion
<!-- <type>bundle</type>-->
```
```suggestion
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/pom.xml:
##########
@@ -134,11 +133,26 @@
<artifactId>bval-jsr</artifactId>
<version>2.0.5</version>
</dependency>
+ <dependency>
+ <groupId>org.codehaus.jackson</groupId>
+ <artifactId>jackson-mapper-asl</artifactId>
+ <version>1.9.13</version>
+ </dependency>
Review Comment:
Is this dependency still required with the refactored classes that use Jackson 2?
##########
pom.xml:
##########
@@ -269,6 +270,17 @@
<artifactId>bcpg-jdk15on</artifactId>
<version>${org.bouncycastle.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.avro</groupId>
+ <artifactId>avro</artifactId>
+ <version>${avro.version}</version>
+<!-- <type>bundle</type>-->
Review Comment:
This comment should be removed.
```suggestion
<!-- <type>bundle</type>-->
```
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org