MartijnVisser commented on code in PR #332:
URL: https://github.com/apache/flink-statefun/pull/332#discussion_r1405970736
##########
pom.xml:
##########
@@ -76,15 +76,28 @@ under the License.
<java.version>1.8</java.version>
<spotless-maven-plugin.version>1.20.0</spotless-maven-plugin.version>
<auto-service.version>1.0-rc6</auto-service.version>
- <protobuf.version>3.7.1</protobuf.version>
- <unixsocket.version>2.3.2</unixsocket.version>
-
<protoc-jar-maven-plugin.version>3.11.1</protoc-jar-maven-plugin.version>
- <flink.version>1.15.2</flink.version>
+ <protobuf.version>3.23.2</protobuf.version>
+ <unixsocket.version>2.6.2</unixsocket.version>
+
<protoc-jar-maven-plugin.version>3.11.4</protoc-jar-maven-plugin.version>
+ <flink.version>1.17.1</flink.version>
<scala.binary.version>2.12</scala.binary.version>
<scala.version>2.12.7</scala.version>
<lz4-java.version>1.8.0</lz4-java.version>
-
<flink-shaded-jackson.version>2.12.4-15.0</flink-shaded-jackson.version>
+
<flink-shaded-jackson.version>2.14.2-17.0</flink-shaded-jackson.version>
Review Comment:
This is the wrong version for Flink 1.17.1
##########
tools/docker/Dockerfile:
##########
@@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-FROM apache/flink:1.15.2-scala_2.12-java8
+FROM flink:1.17.1-scala_2.12-java11
Review Comment:
Statefun still requires Java 8, so I don't think we should bump it as part
of this PR
##########
statefun-flink/statefun-flink-io-bundle/pom.xml:
##########
@@ -90,7 +90,12 @@ under the License.
<dependency>
<groupId>org.apache.flink</groupId>
<artifactId>flink-connector-kinesis</artifactId>
- <version>${flink.version}</version>
+ <version>${flink-connector-kinesis.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-connector-aws-kinesis-streams</artifactId>
+ <version>${flink-connector-aws-kinesis-streams.version}</version>
Review Comment:
Is this necessary for the Flink upgrade itself?
##########
pom.xml:
##########
@@ -234,10 +311,12 @@ under the License.
<outputTarget>
<type>descriptor</type>
<outputDirectory>${basedir}/target/test-classes</outputDirectory>
+ <addSources>main</addSources>
Review Comment:
Is this needed for the 1.171. upgrade?
##########
pom.xml:
##########
@@ -129,7 +147,66 @@ under the License.
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.13.2.2</version>
+ <version>${jackson-databind.version}</version>
+ </dependency>
+ <!--
+ Resolve dependency convergence issue:
+ org.apache.flink:flink-streaming-java:1.17.1 depends on
org.apache.flink:flink-shaded-netty:4.1.82.Final-16.1
+ org.apache.flink:statefun-flink-core:3.4-SNAPSHOT depends on
org.apache.flink:flink-shaded-netty:4.1.70.Final-15.0
+ (via
com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.13.2)
+ -->
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-shaded-netty</artifactId>
+ <version>${flink-shaded-netty.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-core</artifactId>
+ <version>${flink.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-table-common</artifactId>
+ <version>${flink.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-connector-base</artifactId>
+ <version>${flink.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-shaded-force-shading</artifactId>
+ <version>${flink-shaded-force-shading.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.flink</groupId>
+ <artifactId>flink-shaded-jackson</artifactId>
+ <version>${flink-shaded-jackson.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>commons-codec</groupId>
+ <artifactId>commons-codec</artifactId>
+ <version>${commons-codec.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>commons-logging</groupId>
+ <artifactId>commons-logging</artifactId>
+ <version>${commons-logging.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ <version>${slf4j-api.version}</version>
Review Comment:
Why are we adding these? That shouldn't be necessary in an upgrade to 1.17.1
##########
pom.xml:
##########
@@ -76,15 +76,28 @@ under the License.
<java.version>1.8</java.version>
<spotless-maven-plugin.version>1.20.0</spotless-maven-plugin.version>
<auto-service.version>1.0-rc6</auto-service.version>
- <protobuf.version>3.7.1</protobuf.version>
- <unixsocket.version>2.3.2</unixsocket.version>
-
<protoc-jar-maven-plugin.version>3.11.1</protoc-jar-maven-plugin.version>
- <flink.version>1.15.2</flink.version>
+ <protobuf.version>3.23.2</protobuf.version>
+ <unixsocket.version>2.6.2</unixsocket.version>
+
<protoc-jar-maven-plugin.version>3.11.4</protoc-jar-maven-plugin.version>
+ <flink.version>1.17.1</flink.version>
<scala.binary.version>2.12</scala.binary.version>
<scala.version>2.12.7</scala.version>
<lz4-java.version>1.8.0</lz4-java.version>
-
<flink-shaded-jackson.version>2.12.4-15.0</flink-shaded-jackson.version>
+
<flink-shaded-jackson.version>2.14.2-17.0</flink-shaded-jackson.version>
<slf4j-log4j12.version>1.7.32</slf4j-log4j12.version>
+
<flink-connector-kinesis.version>4.1.0-1.17</flink-connector-kinesis.version>
+
<flink-connector-aws-kinesis-streams.version>4.1.0-1.17</flink-connector-aws-kinesis-streams.version>
+ <okhttp.version>3.14.6</okhttp.version>
+
<flink-shaded-netty.version>4.1.82.Final-16.1</flink-shaded-netty.version>
+ <junit.version>4.12</junit.version>
+ <hamcrest-all.version>1.3</hamcrest-all.version>
+ <kryo.version>2.24.0</kryo.version>
+ <jackson-databind.version>2.13.2.2</jackson-databind.version>
+
<flink-shaded-netty.version>4.1.82.Final-16.1</flink-shaded-netty.version>
+
<flink-shaded-force-shading.version>16.1</flink-shaded-force-shading.version>
+ <commons-codec.version>1.15</commons-codec.version>
+ <commons-logging.version>1.2</commons-logging.version>
+ <slf4j-api.version>1.7.36</slf4j-api.version>
Review Comment:
It doesn't appear as we re-use these variables anywhere else, should we just
leave them as they were?
##########
statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/StatefulFunctionsConfig.java:
##########
@@ -265,7 +265,7 @@ public void setEmbedded(boolean embedded) {
*/
public StatefulFunctionsUniverseProvider getProvider(ClassLoader cl) {
try {
- return
InstantiationUtil.deserializeObject(universeInitializerClassBytes, cl, false);
+ return
InstantiationUtil.deserializeObject(universeInitializerClassBytes, cl);
Review Comment:
Is this needed for the Flink upgrade?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
