Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support JDKs 11 & 17 #8901

Merged
merged 9 commits into from
Mar 22, 2022
Merged

Support JDKs 11 & 17 #8901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4f09fef
chore: use mockito for WelcomeMsgUtilsTest
agavra Mar 16, 2022
6d7c8ad
build: support compiling with JDK17
agavra Mar 16, 2022
4b7dc2d
chore: fix some more stuff
agavra Mar 16, 2022
200dee1
chore: update some more stuff to get it working
agavra Mar 17, 2022
7e811a2
chore: fix even more tests
agavra Mar 18, 2022
7e0af22
chore: fix TopicStreamWritingTest for JDK11
agavra Mar 18, 2022
a899239
chore: change a static loading race condition
agavra Mar 18, 2022
8b506b0
feedback
agavra Mar 21, 2022
1645ff3
Update pom.xml
agavra Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: fix some more stuff
  • Loading branch information
@agavra
agavra committed Mar 16, 2022
commit 4b7dc2dfe5ec06cf04c05ce9b8a67e32b0151b1e
2 changes: 1 addition & 1 deletion Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env groovy

dockerfile {
common {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will use the openjdk-11 pool in Jenkins

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

slackChannel = '#ksqldb-quality-oncall'
upstreamProjects = 'confluentinc/schema-registry'
extraDeployArgs = '-Ddocker.skip=true'
Expand Down
12 changes: 12 additions & 0 deletions ksqldb-common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,18 @@
</execution>
</executions>
</plugin>
<plugin>
<!-- Not sure why, but if we allow forks for the failsafe plugin
for this module using JDK17, we have issues with the classloader.
Instead of blocking our migration to JDK17 I've just disabled forks
for this module, which doesn't actually run any tests anyway so it
shouldn't cause the build to slow down -->
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<configuration>
<forkCount>0</forkCount>
</configuration>
</plugin>
</plugins>

<resources>
Expand Down
4 changes: 3 additions & 1 deletion ksqldb-engine/src/test/java/io/confluent/ksql/execution/ExpressionEvaluatorParityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package io.confluent.ksql.execution;

import static org.hamcrest.CoreMatchers.anyOf;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;
Expand Down Expand Up @@ -425,7 +427,7 @@ private void runEvaluator(
= ((RecordProcessingError) errorMessageCaptor.getValue());
if (error.get().getMessage() == null) {
assertThat(processingError.getException().get().getMessage(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CoreMatchers.any(String.class));
anyOf(CoreMatchers.any(String.class), nullValue()));
} else {
assertThat(processingError.getException().get().getMessage(),
containsString(error.get().getMessage()));
Expand Down