Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5067

Make Flink compile with 1.8 Java compiler

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.3.0
    • Component/s: Build System
    • Labels:
      None
    • Environment:

      macOS Sierra 10.12.1, java version "1.8.0_112", Apache Maven 3.3.9

      Description

      Flink fails to compile when using 1.8 as source and target in Maven. There are two types of issue that are both related to the new type inference rules:

      • Call to TypeSerializer.copy method in TupleSerializer.java:112 now resolves to a different overload than before causing a compilation error: [ERROR] /Users/andrey.melentyev/Dev/github.com/apache/flink/flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/TupleSerializer.java:[112,63] incompatible types: void cannot be converted to java.lang.Object
      • A number of unit tests using assertEquals fail to compile:
        [ERROR] /Users/andrey.melentyev/Dev/github.com/apache/flink/flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java:[50,25] reference to assertEquals is ambiguous
        [ERROR] both method assertEquals(long,long) in org.junit.Assert and method assertEquals(java.lang.Object,java.lang.Object) in org.junit.Assert match

      In both of the above scenarios explicitly casting one of the arguments helps the compiler to resolve overloaded method call correctly.

      It is possible to maintain Flink's code base in a state when it can be built by both 1.7 and 1.8. For this purpose we need minor code fixes and an automated build in Travis to keep the new good state.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user melentye opened a pull request:

          https://github.com/apache/flink/pull/2804

          FLINK-5067 Make Flink compile with 1.8 Java compiler

          • New type inference rules changed which overloaded methods are picked.
          • Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified.
          • Introduced Java 8 build for Travis CI

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/melentye/flink FLINK-5067-make-flink-compile-with-java8-compiler

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/2804.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 #2804


          commit f07215eed4269826e57136fa30560a34b8409d34
          Author: Andrey Melentyev <andrey.melentyev@klarna.com>
          Date: 2016-11-13T16:42:17Z

          FLINK-5067 Java 8 build fixes

          • New type inference rules changed which overloaded methods are picked.
          • Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified.
          • Introduced Java 8 build for Travis CI

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user melentye opened a pull request: https://github.com/apache/flink/pull/2804 FLINK-5067 Make Flink compile with 1.8 Java compiler New type inference rules changed which overloaded methods are picked. Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified. Introduced Java 8 build for Travis CI You can merge this pull request into a Git repository by running: $ git pull https://github.com/melentye/flink FLINK-5067 -make-flink-compile-with-java8-compiler Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2804.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 #2804 commit f07215eed4269826e57136fa30560a34b8409d34 Author: Andrey Melentyev <andrey.melentyev@klarna.com> Date: 2016-11-13T16:42:17Z FLINK-5067 Java 8 build fixes New type inference rules changed which overloaded methods are picked. Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified. Introduced Java 8 build for Travis CI
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88241945

          — Diff: flink-tests/src/test/java/org/apache/flink/test/windowing/sessionwindows/SessionWindowITCase.java —
          @@ -122,11 +122,11 @@ private void runTest(

          // check that overall event counts match with our expectations. remember that late events within lateness will
          // each trigger a window!

          • Assert.assertEquals(
          • (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION,
            + Assert.assertEquals(Long.valueOf(
            + (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION),
            result.getAccumulatorResult(SESSION_COUNTER_ON_TIME_KEY));
          • Assert.assertEquals(
          • NUMBER_OF_SESSIONS * (LATE_EVENTS_PER_SESSION * (LATE_EVENTS_PER_SESSION + 1) / 2),
            + Assert.assertEquals(Long.valueOf(
              • End diff –

          I think unboxing the second argument would be nicer than boxing the first

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88241945 — Diff: flink-tests/src/test/java/org/apache/flink/test/windowing/sessionwindows/SessionWindowITCase.java — @@ -122,11 +122,11 @@ private void runTest( // check that overall event counts match with our expectations. remember that late events within lateness will // each trigger a window! Assert.assertEquals( (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION, + Assert.assertEquals(Long.valueOf( + (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION), result.getAccumulatorResult(SESSION_COUNTER_ON_TIME_KEY)); Assert.assertEquals( NUMBER_OF_SESSIONS * (LATE_EVENTS_PER_SESSION * (LATE_EVENTS_PER_SESSION + 1) / 2), + Assert.assertEquals(Long.valueOf( End diff – I think unboxing the second argument would be nicer than boxing the first
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88242077

          — Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java —
          @@ -35,7 +35,7 @@
          @Test
          public void testAccumulator() {
          try {

          • final int NUM_ELEMENTS = 100;
            + final Integer NUM_ELEMENTS = 100;
              • End diff –

          Can we stay, whereever possible, with primitive types? Rather unbox the type at a different place?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88242077 — Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java — @@ -35,7 +35,7 @@ @Test public void testAccumulator() { try { final int NUM_ELEMENTS = 100; + final Integer NUM_ELEMENTS = 100; End diff – Can we stay, whereever possible, with primitive types? Rather unbox the type at a different place?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88242410

          — Diff: pom.xml —
          @@ -96,7 +96,7 @@ under the License.
          <slf4j.version>1.7.7</slf4j.version>
          <guava.version>18.0</guava.version>
          <akka.version>2.3.7</akka.version>

          • <java.version>1.7</java.version>
            + <java.minor.version>1.7</java.minor.version>
              • End diff –

          Does this make a difference, or is that mainly a naming preference?
          It it is only about naming, I would prefer to undo this change.
          Call it "being conservative" with `pom.xml` changes - they have subtle implication son release scripts, etc.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88242410 — Diff: pom.xml — @@ -96,7 +96,7 @@ under the License. <slf4j.version>1.7.7</slf4j.version> <guava.version>18.0</guava.version> <akka.version>2.3.7</akka.version> <java.version>1.7</java.version> + <java.minor.version>1.7</java.minor.version> End diff – Does this make a difference, or is that mainly a naming preference? It it is only about naming, I would prefer to undo this change. Call it "being conservative" with `pom.xml` changes - they have subtle implication son release scripts, etc.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user melentye commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88331670

          — Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java —
          @@ -35,7 +35,7 @@
          @Test
          public void testAccumulator() {
          try {

          • final int NUM_ELEMENTS = 100;
            + final Integer NUM_ELEMENTS = 100;
              • End diff –

          Absolutely!

          Show
          githubbot ASF GitHub Bot added a comment - Github user melentye commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88331670 — Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java — @@ -35,7 +35,7 @@ @Test public void testAccumulator() { try { final int NUM_ELEMENTS = 100; + final Integer NUM_ELEMENTS = 100; End diff – Absolutely!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user melentye commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88331708

          — Diff: flink-tests/src/test/java/org/apache/flink/test/windowing/sessionwindows/SessionWindowITCase.java —
          @@ -122,11 +122,11 @@ private void runTest(

          // check that overall event counts match with our expectations. remember that late events within lateness will
          // each trigger a window!

          • Assert.assertEquals(
          • (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION,
            + Assert.assertEquals(Long.valueOf(
            + (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION),
            result.getAccumulatorResult(SESSION_COUNTER_ON_TIME_KEY));
          • Assert.assertEquals(
          • NUMBER_OF_SESSIONS * (LATE_EVENTS_PER_SESSION * (LATE_EVENTS_PER_SESSION + 1) / 2),
            + Assert.assertEquals(Long.valueOf(
              • End diff –

          Sure thing, fixed

          Show
          githubbot ASF GitHub Bot added a comment - Github user melentye commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88331708 — Diff: flink-tests/src/test/java/org/apache/flink/test/windowing/sessionwindows/SessionWindowITCase.java — @@ -122,11 +122,11 @@ private void runTest( // check that overall event counts match with our expectations. remember that late events within lateness will // each trigger a window! Assert.assertEquals( (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION, + Assert.assertEquals(Long.valueOf( + (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION), result.getAccumulatorResult(SESSION_COUNTER_ON_TIME_KEY)); Assert.assertEquals( NUMBER_OF_SESSIONS * (LATE_EVENTS_PER_SESSION * (LATE_EVENTS_PER_SESSION + 1) / 2), + Assert.assertEquals(Long.valueOf( End diff – Sure thing, fixed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user melentye commented on the issue:

          https://github.com/apache/flink/pull/2804

          @StephanEwen I initially introduced a new Travis profile so that the coverage would be strictly improved, i.e. all old combinations tests plus the new ones. But I am also fine reusing some of the existing profiles too, PR updated.

          Show
          githubbot ASF GitHub Bot added a comment - Github user melentye commented on the issue: https://github.com/apache/flink/pull/2804 @StephanEwen I initially introduced a new Travis profile so that the coverage would be strictly improved, i.e. all old combinations tests plus the new ones. But I am also fine reusing some of the existing profiles too, PR updated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user melentye commented on a diff in the pull request:

          https://github.com/apache/flink/pull/2804#discussion_r88332568

          — Diff: pom.xml —
          @@ -96,7 +96,7 @@ under the License.
          <slf4j.version>1.7.7</slf4j.version>
          <guava.version>18.0</guava.version>
          <akka.version>2.3.7</akka.version>

          • <java.version>1.7</java.version>
            + <java.minor.version>1.7</java.minor.version>
              • End diff –

          I actually prefer java.version for it's conciseness and I share the conservative tune in regards to unnecessary pom.xml changes. But in this case I did it on purpose, long explanation follows.

          The java.version is passed by -Djava.version=1.8 argument to mvn which seems to be Maven-recommended (or even the only one supported) way of redefining the property from command-line. An inconvenient side-effect is that this property is then propagated down until the JVM that executes the tests and causes java.lang.RuntimeException: Unexpected version format: 1.8 at org.apache.hadoop.hbase.util.ClassSize.<clinit>(ClassSize.java:119) starting in org.apache.flink.addons.hbase.TableInputFormatITCase (won't bore you with the complete stack trace). Since this is third party code, we can't really fix it in Flink source. Therefore I thought to rename the property instead to avoid the clash.

          There's a way to keep the java.version property name as is: we can use a maven profile to set java.version. In this case when you activate the profile with -P argument to mvn, the java.version will not be propagated as a system property to the JVM running the tests and won't cause problems.

          I've noticed that there's an existing profile called "jdk8" which sounds relevant. But it's currently automatically activated if you're running Maven with Java 8 so that could turn out to be a surprise for the maintainers to realize that they suddenly are building 1.8 target bytecode. The profile doesn't do much though so I am questioning if it really needs to be auto-activated. I think it'll be cleaner to stop auto-activating it and reuse it for the above purpose. See proposal in the PR update.

          Show
          githubbot ASF GitHub Bot added a comment - Github user melentye commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88332568 — Diff: pom.xml — @@ -96,7 +96,7 @@ under the License. <slf4j.version>1.7.7</slf4j.version> <guava.version>18.0</guava.version> <akka.version>2.3.7</akka.version> <java.version>1.7</java.version> + <java.minor.version>1.7</java.minor.version> End diff – I actually prefer java.version for it's conciseness and I share the conservative tune in regards to unnecessary pom.xml changes. But in this case I did it on purpose, long explanation follows. The java.version is passed by -Djava.version=1.8 argument to mvn which seems to be Maven-recommended (or even the only one supported) way of redefining the property from command-line. An inconvenient side-effect is that this property is then propagated down until the JVM that executes the tests and causes java.lang.RuntimeException: Unexpected version format: 1.8 at org.apache.hadoop.hbase.util.ClassSize.<clinit>(ClassSize.java:119) starting in org.apache.flink.addons.hbase.TableInputFormatITCase (won't bore you with the complete stack trace). Since this is third party code, we can't really fix it in Flink source. Therefore I thought to rename the property instead to avoid the clash. There's a way to keep the java.version property name as is: we can use a maven profile to set java.version. In this case when you activate the profile with -P argument to mvn, the java.version will not be propagated as a system property to the JVM running the tests and won't cause problems. I've noticed that there's an existing profile called "jdk8" which sounds relevant. But it's currently automatically activated if you're running Maven with Java 8 so that could turn out to be a surprise for the maintainers to realize that they suddenly are building 1.8 target bytecode. The profile doesn't do much though so I am questioning if it really needs to be auto-activated. I think it'll be cleaner to stop auto-activating it and reuse it for the above purpose. See proposal in the PR update.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/2804

          Looks good!

          Just curious, you mentioned that using the `java.version` variable clashes with how it is used by the JVM. Is that not an issue any more?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2804 Looks good! Just curious, you mentioned that using the `java.version` variable clashes with how it is used by the JVM. Is that not an issue any more?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user melentye commented on the issue:

          https://github.com/apache/flink/pull/2804

          @StephanEwen it seems that the semantics is different when java.version is just defined in pom.xml versus being overriden with -Djava.version=x.y.z argument passed to mvn.

          org.apache.hadoop.hbase.util.ClassSize expects a very specific format of java.version system property. But when java.version is only defined in pom.xml then it doesn't actually become a system property unlike in case of using -Djava.version

          Show
          githubbot ASF GitHub Bot added a comment - Github user melentye commented on the issue: https://github.com/apache/flink/pull/2804 @StephanEwen it seems that the semantics is different when java.version is just defined in pom.xml versus being overriden with -Djava.version=x.y.z argument passed to mvn. org.apache.hadoop.hbase.util.ClassSize expects a very specific format of java.version system property. But when java.version is only defined in pom.xml then it doesn't actually become a system property unlike in case of using -Djava.version
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rmetzger commented on the issue:

          https://github.com/apache/flink/pull/2804

          Looks good to merge.

          I'm currently verifying the change, then, I'll merge it: https://travis-ci.org/rmetzger/flink/builds/207494237

          Show
          githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/2804 Looks good to merge. I'm currently verifying the change, then, I'll merge it: https://travis-ci.org/rmetzger/flink/builds/207494237
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/2804

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/2804
          Hide
          rmetzger Robert Metzger added a comment -
          Show
          rmetzger Robert Metzger added a comment - Resolved in master for 1.3 in http://git-wip-us.apache.org/repos/asf/flink/commit/dc00fb0c
          Hide
          rmetzger Robert Metzger added a comment -

          This JIRA disabled the automatic profile activation for the java8 module, which caused the snapshot deployment on jenkins to fail.
          This commit fixes the snapshot deployment on Jenkins: http://git-wip-us.apache.org/repos/asf/flink/commit/64c7b119

          Show
          rmetzger Robert Metzger added a comment - This JIRA disabled the automatic profile activation for the java8 module, which caused the snapshot deployment on jenkins to fail. This commit fixes the snapshot deployment on Jenkins: http://git-wip-us.apache.org/repos/asf/flink/commit/64c7b119

            People

            • Assignee:
              melentye Andrey Melentyev
              Reporter:
              melentye Andrey Melentyev
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development