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.

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

Java 21 compatibility #14502

Open
gianm opened this issue Jun 29, 2023 · 10 comments
Open

Java 21 compatibility #14502

gianm opened this issue Jun 29, 2023 · 10 comments

Comments

@gianm
Copy link
Contributor

gianm commented Jun 29, 2023

Java 21 is the next LTS release, expected September 2023. We'd like to officially support this.

It isn't out yet, but we can start with Java 20.

@clintropolis
Copy link
Member

clintropolis commented Jun 29, 2023

I'll share my notes since I've been slowly working my way through this!

Equalsverifier based tests fail with:

Java 20 (64) is not supported by the current version of Byte Buddy which officially supports Java 18 (62) - update Byte Buddy or set net.bytebuddy.experimental as a VM property

which is resolved by manually adding the newest version of byte-buddy to the pom (I also updated both equalsverifier and mockito to latest versions which also have transient dependencies on byte-buddy, but that wasn't enough to fix on its own).

Before updating easymock we also see some failures of this form

[ERROR] org.apache.druid.segment.writeout.FileWriteOutBytesTest.writeBufferSmallerThanCapacityShouldIncrementSizeCorrectly  Time elapsed: 0 s  <<< ERROR!
java.lang.IllegalArgumentException: Unsupported class file major version 64

which go away after updating easymock, however I am still seeing a failure on the latest version of easymock:

[ERROR] org.apache.druid.query.aggregation.constant.LongConstantBufferAggregatorTest.testAggregate  Time elapsed: 0.018 s  <<< ERROR!
java.lang.IllegalArgumentException: Could not create type
	at org.easymock.bytebuddy.TypeCache.findOrInsert(TypeCache.java:170)
	at org.easymock.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:399)
	at org.easymock.internal.ClassProxyFactory.createProxy(ClassProxyFactory.java:136)
	at org.easymock.internal.MocksControl.createMock(MocksControl.java:108)
	at org.easymock.internal.MocksControl.createMock(MocksControl.java:81)
	at org.easymock.IMocksControl.mock(IMocksControl.java:44)
	at org.easymock.EasyMock.mock(EasyMock.java:70)
...
Caused by: java.lang.IllegalStateException: java.lang.IncompatibleClassChangeError: class org.easymock.mocks.ByteBuffer$$$EasyMock$1 cannot inherit from sealed class java.nio.ByteBuffer

which is as far as i've got so far, I think maybe easymock hasn't been updated to be cool with java 20, as https://github.com/easymock/easymock/releases has an older version of bytebuddy than i added in my tests to our pom

@gianm
Copy link
Contributor Author

gianm commented Jun 29, 2023

For LongConstantBufferAggregatorTest specifically, I think we can replace buffer = EasyMock.mock(ByteBuffer.class) with buffer = null. It's just verifying that the LongConstantBufferAggregator is not calling any methods on buffer. Can't call methods on null 😉

@gianm
Copy link
Contributor Author

gianm commented Jun 29, 2023

Want to make a branch on apache/druid that various of us can commit to? Once things are buildable someone can make a PR from it. At that point I would suggest running most tests on 8 (oldest supported), 17 (latest LTS supported), and 20.

@clintropolis
Copy link
Member

Yeah, will try to do later today 👍

fwiw druid itself seems to run fine in java 20, i've been using it for all of my dev work running a local cluster for a couple of months now (though disclaimer i mainly use indexers instead of middle managers since easier to debug stuff), so i think it is just test problems.

I let some more tests run while i was eating lunch and the next failure is in druid-server:

[ERROR] org.apache.druid.curator.CuratorModuleTest.exitsJvmWhenMaxRetriesExceeded  Time elapsed: 0 s  <<< ERROR!
java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
	at java.base/java.lang.System.setSecurityManager(System.java:429)
	at org.junit.contrib.java.lang.system.ProvideSecurityManager.before(ProvideSecurityManager.java:39)

and later in druid-sql are some failures like:

[ERROR] org.apache.druid.sql.calcite.rule.DruidLogicalValuesRuleTest$GetValueFromLiteralSimpleTypesTest.testGetValueFromLiteral[BIGINT, class java.lang.Long]  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError: 
Unable to mock the literal for test.
Exception: java.lang.NoSuchFieldException: value
	at org.junit.Assert.fail(Assert.java:89)
	at org.apache.druid.sql.calcite.rule.DruidLogicalValuesRuleTest$GetValueFromLiteralSimpleTypesTest.makeLiteral(DruidLogicalValuesRuleTest.java:109)
	at org.apache.druid.sql.calcite.rule.DruidLogicalValuesRuleTest$GetValueFromLiteralSimpleTypesTest.testGetValueFromLiteral(DruidLogicalValuesRuleTest.java:91)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)

@gianm
Copy link
Contributor Author

gianm commented Jun 29, 2023

For DruidLogicalValuesRuleTest, we can remove the mocks from makeLiteral and make real literals instead:

    private static RexLiteral makeLiteral(Comparable<?> val, SqlTypeName typeName)
    {
      return (RexLiteral) new RexBuilder(DruidTypeSystem.TYPE_FACTORY).makeLiteral(
          typeName == SqlTypeName.DECIMAL && val != null ? new BigDecimal(String.valueOf(val)) : val,
          DruidTypeSystem.TYPE_FACTORY.createSqlType(typeName),
          false
      );
    }

@gianm
Copy link
Contributor Author

gianm commented Jun 30, 2023

The CuratorModuleTest case exitsJvmWhenMaxRetriesExceeded relies on NoExitSecurityManager to trap System.exit. Security managers are being removed gradually from Java 18+ (https://openjdk.org/jeps/411). The test will need to be rewritten or deleted.

IMO a good option would be to adjust CuratorModuleTest to subclass CuratorModule and override shutdown(lifecycle). The overridden method should set an AtomicBoolean in the subclass rather than calling System.exit(1). Then, the test case should check the AtomicBoolean rather than using ExpectedSystemExit.

@xvrl
Copy link
Member

xvrl commented Sep 19, 2023

I was able to make unit tests pass in #15014. The biggest issue I see is that we need mockito 5.x to run tests with Java 21, however that version no longer supports Java 8. We might be able to do some workaround where we run tests with different mockito versions and restrict ourselves to the shared APIs, but that is also somewhat tricky since mockito-inline is now merged into mockito-core.

@clintropolis
Copy link
Member

The biggest issue I see is that we need mockito 5.x to run tests with Java 21, however that version no longer supports Java 8

Now that we've dropped support for Hadoop 2, maybe we can consider doing the same for Java 8? Afaik that was the primary reason we needed to keep supporting it... though we'd need to decide on a new minimum version.

@xvrl
Copy link
Member

xvrl commented Sep 19, 2023

IMO we should only target LTS Java versions, so 11 should be the new minimum version

@gianm
Copy link
Contributor Author

gianm commented Sep 20, 2023

Current versions of Kafka and Spark still support Java 8; I'd rather not be on the vanguard of dropping support, unless we get something big in return.

Kafka has deprecated it for removal in Kafka 4.0. Maybe we can do something similar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants