Support Flink 1.17.1 #332
Support Flink 1.17.1 #332
Conversation
gfmio
force-pushed
the
support-flink-1.17.1
branch
from
1a6ae6e
to
6478acd
Compare
…n into support-flink-1.17.1
|
@gfmio Thanks a lot for the PRs, but can you please follow the code contribution process as explained on https://flink.apache.org/how-to-contribute/contribute-code/ ? Specifically talking about filing a Jira, following the right commit structure and naming conventions etc. |
|
Apologies, sure, will do. |
|
Hey, any update on this? @MartijnVisser @gfmio |
There was a problem hiding this comment.
@gfmio I've looked over the PR, but it looks like there are changes in here which aren't necessary for the upgrade to Flink 1.17.1 and should be a separate ticket or at the very least, a separate commit. Could you rebase and make the necessary changes?
| <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> |
There was a problem hiding this comment.
This is the wrong version for Flink 1.17.1
| <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> |
There was a problem hiding this comment.
It doesn't appear as we re-use these variables anywhere else, should we just leave them as they were?
| <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> |
There was a problem hiding this comment.
Why are we adding these? That shouldn't be necessary in an upgrade to 1.17.1
| @@ -234,10 +311,12 @@ under the License. | |||
| <outputTarget> | |||
| <type>descriptor</type> | |||
| <outputDirectory>${basedir}/target/test-classes</outputDirectory> | |||
| <addSources>main</addSources> | |||
There was a problem hiding this comment.
Is this needed for the 1.171. upgrade?
| @@ -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); | |||
There was a problem hiding this comment.
Is this needed for the Flink upgrade?
| <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> |
There was a problem hiding this comment.
Is this necessary for the Flink upgrade itself?
| @@ -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 | |||
There was a problem hiding this comment.
Statefun still requires Java 8, so I don't think we should bump it as part of this PR
|
@gfmio For context, I've made MartijnVisser#3 which would probably be sufficient |
|
hey, any news on this? I used your branch @MartijnVisser and the changes are working as expected, even with flink 1.19.1 (I haven't tested any later versions). |
This PR updates the project to use and be compatible with Flink 1.17.1.
We've been running this in production for a few months now without any issues.