Support JDKs 11 & 17 #8901

agavra · 2022-03-16T20:59:22Z

Description

Make the changes required to support compilation with JDK11/17. Additional notes inline. Note that we will continue to compile use JDK8 for now.

Testing done

Ran all the tests using:

Apache Maven 3.8.5 (3599d3414f046de2324203b78ddcf9b5e4388aa0)
Maven home: /usr/local/Cellar/maven/3.8.5/libexec
Java version: 17.0.2, vendor: Homebrew, runtime: /usr/local/Cellar/openjdk/17.0.2/libexec/openjdk.jdk/Contents/Home
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "12.2.1", arch: "x86_64", family: "mac"

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

agavra · 2022-03-16T21:00:01Z

ksqldb-common/src/test/java/io/confluent/ksql/QueryApplicationIdTest.java

@@ -12,6 +12,7 @@
 * WARRANTIES OF ANY KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations under the License.
 */
+package io.confluent.ksql;


the surefire plugin wasn't happy with this class not being part of any module

Neither am I! Thanks for the fix.

agavra · 2022-03-16T21:00:25Z

ksqldb-common/src/test/java/io/confluent/ksql/util/WelcomeMsgUtilsTest.java


-@RunWith(EasyMockRunner.class)
+@RunWith(MockitoJUnitRunner.class)


for some reason EasyMock wasn't working as well after the JDK upgrade. instead of trying to figure it out I just replaced the failing EasyMock tests with Mockito

I assume that we're all in on Mockito at this point anyway. I haven't seen anyone write an Easymock test and assume there aren't many.

agavra · 2022-03-16T21:03:48Z

ksqldb-engine/src/test/java/io/confluent/ksql/execution/ExpressionEvaluatorParityTest.java

@@ -424,7 +424,8 @@ private void runEvaluator(
      RecordProcessingError processingError
          = ((RecordProcessingError) errorMessageCaptor.getValue());
      if (error.get().getMessage() == null) {
-        assertThat(processingError.getException().get().getMessage(), nullValue());
+        assertThat(processingError.getException().get().getMessage(),


NullPointerExceptions in JDK14+ now have better error messages indicating what caused the NPE. While that's fantastic, it causes a few of the tests to fail that compare the expected error message

Thanks. Is the new matcher meaningful? I.e., we're checking that the message, which must either be a String or a null, is either a String or a null. It seems like we could equivalently make no assertion, right?

fair enough! I don't think the assertion is particularly helpful in either way

agavra · 2022-03-16T21:03:58Z

ksqldb-execution/src/main/java/io/confluent/ksql/logging/processing/RecordProcessingError.java

@@ -149,4 +149,13 @@ private static String serializeRow(final GenericRow record) {
      return null;
    }
  }
+
+  @Override
+  public String toString() {


this just helped debug things

agavra · 2022-03-16T21:05:20Z

...tional-tests/src/main/java/io/confluent/ksql/test/serde/json/ValueSpecJsonSerdeSupplier.java

    this.useSchemas = useSchemas;
+    mapper = (boolean) (properties.getOrDefault("use.big.decimal.for.floats", true))


I had to update geodistance tests to using floating point comparisons instead of decimals since it seems like some of the floating point operations have changed precision in the different JDKs. This gives us that flexibility. The geodistance test used doubles (not decimals) anyway.

Some operations changed precision but we're using them anyway? That seems kind of crazy. I thought most of the operations are well defined standard operations on IEEE floats.

Is this really necessary here? It seems like you're disabling BigDecimal only so that the actual value will be a Double, so that the +/- 0.000001 logic below will take effect, but can't you also implement that fudge factor without disabling BigDecimal?

I'm only concerned because it seems not at all obvious why that's an important config in the tests that it's in now, nor why we should or should not include it in tests in the future.

but can't you also implement that fudge factor without disabling BigDecimal

we should not be fudging comparisons with decimals, as that would explicitly defeat their purpose! I'm not sure I'm understanding your question?

Thanks, @agavra , we just synced offline about this. Just to document what I was getting at was that it seems a bit abstract to say "I'm going to disable BigDecimal here" for the underlying reason that I happen to know that disabling BigDecimal means that the test suite will fall back to Double, and further that if it falls back to Double, it'll only compare with 6 digits of precision, when all I'm really after is the 6-digit precision.

In contrast, if we add a config that says "compute result equality +/- 0.000001", it'll be crystal clear what the test spec is doing. Then, we wouldn't need that explanatory comment in the spec, it'll be clear which tests are precise or not (regardless of what type they use), it'll be clear for future devs how to adjust their tests to account for precision errors, and we won't be inadvertantly relaxing the validations for all our other tests.

I updated this to use.exact.numeric.comparsion=false, I ultimately buy your argument :)

Thanks for hearing me out :) Sorry it took me a while to find the words for my feelings.

agavra · 2022-03-16T21:05:38Z

...db-functional-tests/src/main/java/io/confluent/ksql/test/tools/ExpectedRecordComparator.java

@@ -140,7 +140,7 @@ private static boolean compareNumber(final Object actualValue, final JsonNode ex
    }

    if (actualValue instanceof Double) {
-      return expected.doubleValue() == (Double) actualValue;
+      return Math.abs(expected.doubleValue() - (Double) actualValue) < 0.000_001;


six digits of comparison is pretty good for floating point math

Could use http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/number/IsCloseTo.html

jnh5y · 2022-03-16T22:17:04Z

Do we want to run the Java 11 and Java 17 builds in parallel in CI? That may be possible... (without causing the tests to light on fire)

agavra · 2022-03-16T23:08:44Z

@jnh5y - I just updated the Jenkinsfile to run with jdk11. I'm not sure we have images yet that run with JDK17 :/ we can consider that as a follow up.

FWIW, I am having some issues with JDK17 and the failsafe plugin 😢 maybe you have some ideas. When it runs with forks there are NoClassDefFoundErrors but when it runs without forks it passes fine...

agavra · 2022-03-18T17:51:41Z

Jenkinsfile

@@ -1,6 +1,6 @@
 #!/usr/bin/env groovy

-dockerfile {
+common {


this will use the openjdk-11 pool in Jenkins

Is it possible to add a comment? It seems pretty subtle.

I don't think a comment is super helpful here, generally tools wanted us to move off of dockerfile and onto common and common conveniently uses jdk11 pool. You can specify nodeLabel to change the node pool that you use.

agavra · 2022-03-18T17:52:07Z

pom.xml

-                    <compilerVersion>1.8</compilerVersion>
-                    <source>1.8</source>
-                    <target>1.8</target>
+                    <compilerVersion>11</compilerVersion>


I tested with both 11 and 17, using 11 here because that's what hte Jenkins nodes compile against

Are you sure you want to change this? Kafka still uses "--release 8" and it supports 8, 11 and 17. The changes here drop support for 8.

@MichaelDrogalis thoughts? the team is eager to use JDK11 features, but I see product relevance for leaving jdk8 support

Kafka will probably do that next year, but it depends on the reaction from users. This is the Kafka KIP https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=181308223

thanks, updated the PR to remain with JDK8 for compilation

agavra · 2022-03-18T17:52:32Z

pom.xml

@@ -128,7 +128,7 @@
        <really.executable.jar.version>1.5.0</really.executable.jar.version>
        <generext.version>1.0.2</generext.version>
        <avro.random.generator.version>0.2.2</avro.random.generator.version>
-        <apache.curator.version>2.9.0</apache.curator.version>
+        <apache.curator.version>5.2.0</apache.curator.version>


needed this to prevent some reflection issues

AlanConfluent · 2022-03-21T16:49:17Z

ksqldb-common/src/test/java/io/confluent/ksql/util/WelcomeMsgUtilsTest.java


-@RunWith(EasyMockRunner.class)
+@RunWith(MockitoJUnitRunner.class)


I assume that we're all in on Mockito at this point anyway. I haven't seen anyone write an Easymock test and assume there aren't many.

AlanConfluent · 2022-03-21T16:54:15Z

...tional-tests/src/main/java/io/confluent/ksql/test/serde/json/ValueSpecJsonSerdeSupplier.java

    this.useSchemas = useSchemas;
+    mapper = (boolean) (properties.getOrDefault("use.big.decimal.for.floats", true))


Some operations changed precision but we're using them anyway? That seems kind of crazy. I thought most of the operations are well defined standard operations on IEEE floats.

AlanConfluent · 2022-03-21T16:56:12Z

...db-functional-tests/src/main/java/io/confluent/ksql/test/tools/ExpectedRecordComparator.java

@@ -140,7 +140,7 @@ private static boolean compareNumber(final Object actualValue, final JsonNode ex
    }

    if (actualValue instanceof Double) {
-      return expected.doubleValue() == (Double) actualValue;
+      return Math.abs(expected.doubleValue() - (Double) actualValue) < 0.000_001;


Could use http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/number/IsCloseTo.html

vvcephei

Nice work, @agavra !

I left some comments, but feel free to take them or leave them.

vvcephei · 2022-03-21T18:11:41Z

Jenkinsfile

@@ -1,6 +1,6 @@
 #!/usr/bin/env groovy

-dockerfile {
+common {


Is it possible to add a comment? It seems pretty subtle.

vvcephei · 2022-03-21T18:15:05Z

ksqldb-engine/src/test/java/io/confluent/ksql/execution/ExpressionEvaluatorParityTest.java

@@ -424,7 +424,8 @@ private void runEvaluator(
      RecordProcessingError processingError
          = ((RecordProcessingError) errorMessageCaptor.getValue());
      if (error.get().getMessage() == null) {
-        assertThat(processingError.getException().get().getMessage(), nullValue());
+        assertThat(processingError.getException().get().getMessage(),


Thanks. Is the new matcher meaningful? I.e., we're checking that the message, which must either be a String or a null, is either a String or a null. It seems like we could equivalently make no assertion, right?

vvcephei · 2022-03-21T18:15:47Z

ksqldb-execution/src/main/java/io/confluent/ksql/logging/processing/RecordProcessingError.java

@@ -149,4 +149,13 @@ private static String serializeRow(final GenericRow record) {
      return null;
    }
  }
+
+  @Override
+  public String toString() {


vvcephei · 2022-03-21T19:11:09Z

...tional-tests/src/main/java/io/confluent/ksql/test/serde/json/ValueSpecJsonSerdeSupplier.java

    this.useSchemas = useSchemas;
+    mapper = (boolean) (properties.getOrDefault("use.big.decimal.for.floats", true))


Is this really necessary here? It seems like you're disabling BigDecimal only so that the actual value will be a Double, so that the +/- 0.000001 logic below will take effect, but can't you also implement that fudge factor without disabling BigDecimal?

I'm only concerned because it seems not at all obvious why that's an important config in the tests that it's in now, nor why we should or should not include it in tests in the future.

vvcephei · 2022-03-21T19:19:44Z

...p/src/test/java/io/confluent/ksql/rest/server/resources/streaming/TopicStreamWriterTest.java

+    /**
+     * In JDK 13+ the behavior of PrintStream was changed so that newlines are flushed
+     * in the same write as the line itself when written using `println()` method. This
+     * method attempts to support both behaviors.
+     *
+     * @see <a href=
+     *  "https://github.com/openjdk/jdk/commit/346018251f22187b0508e11edd13833a3074c0cc">
+     *  8215412: Optimize PrintStream.println methods</a>
+     */


Woah, this is pretty hairy. If this works, then all good, but just a quick question: can we avoid all this by just concatenating all the elements into one string and splitting on newlines?

yeah, we could do that 😬 😅

agavra · 2022-03-21T21:50:31Z

ksqldb-test-util/src/main/resources/log4j.properties

@@ -20,6 +20,7 @@ log4j.appender.stdout.layout.ConversionPattern=[%d] %p %m (%c:%L)%n

 # help debug a flaky test
 log4j.logger.io.confluent.ksql.api.server=DEBUG
+log4j.logger.io.confluent.rest.server.HeartbeatAgent=DEBUG


I was going to add this in a different PR to help debug a flaky test but forgot to create a branch and it snuck in. hopefully not a problem

sounds good

agavra added 2 commits March 16, 2022 10:14

chore: use mockito for WelcomeMsgUtilsTest

4f09fef

build: support compiling with JDK17

6d7c8ad

agavra commented Mar 16, 2022

View reviewed changes

agavra marked this pull request as ready for review March 16, 2022 21:06

agavra requested a review from a team as a code owner March 16, 2022 21:06

chore: fix some more stuff

4b7dc2d

agavra added 4 commits March 17, 2022 11:50

chore: update some more stuff to get it working

200dee1

chore: fix even more tests

7e811a2

chore: fix TopicStreamWritingTest for JDK11

7e0af22

chore: change a static loading race condition

a899239

agavra commented Mar 21, 2022

View reviewed changes

agavra changed the title ~~Support JDK17~~ Compile with JDK11 and support JDK17 Mar 21, 2022

AlanConfluent approved these changes Mar 21, 2022

View reviewed changes

vvcephei approved these changes Mar 21, 2022

View reviewed changes

feedback

8b506b0

agavra commented Mar 21, 2022

View reviewed changes

vvcephei approved these changes Mar 22, 2022

View reviewed changes

Update pom.xml

1645ff3

agavra changed the title ~~Compile with JDK11 and support JDK17~~ Support JDKs 11 & 17 Mar 22, 2022

agavra merged commit 0833de7 into confluentinc:master Mar 22, 2022

agavra deleted the jdk17 branch March 22, 2022 18:20


		@RunWith(EasyMockRunner.class)
		@RunWith(MockitoJUnitRunner.class)

		this.useSchemas = useSchemas;
		mapper = (boolean) (properties.getOrDefault("use.big.decimal.for.floats", true))

Support JDKs 11 & 17 #8901

Support JDKs 11 & 17 #8901

Conversation

agavra commented Mar 16, 2022 • edited Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnh5y commented Mar 16, 2022

agavra commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvcephei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra commented Mar 16, 2022 •

edited

Loading

ijuma Mar 22, 2022 •

edited

Loading