chore(deps): Upgrade jol-core to latest#27128
chore(deps): Upgrade jol-core to latest#27128mblanco-denodo wants to merge 1 commit intoprestodb:masterfrom
Conversation
6871e9e to
a3a286e
Compare
|
Solves: #24005 |
db38548 to
2883ece
Compare
2883ece to
55b6804
Compare
ZacBlanco
left a comment
There was a problem hiding this comment.
Thanks for picking this up! I do have a concern about bumping this module's bytecode level. If it doesn't fall into the client's dependency tree and cause it to require JDK 17 then I think it's ok. Otherwise, the changes LGTM
Can you also squash your commits? Thanks!
|
|
||
| <properties> | ||
| <air.main.basedir>${project.parent.basedir}</air.main.basedir> | ||
| <project.build.targetJdk>8</project.build.targetJdk> |
There was a problem hiding this comment.
Sorry I haven't looked closer, but will upgrading the bytecode level on this module affect the client or presto-on-spark? The client likely needs to remain JDK 8 compatible.
There was a problem hiding this comment.
The client needs to be compiled for java 17 as the related airlift dependencies had been bumped to 17 and previously merged. You can review the related PRs in the thread before that have already been approved and merged if there's anything that we need to roollback. I'll rollback the targetJdk for the presto-bytecode/pom.xml in the meantime
## Description: To update jol-core some other elements need to be upgraded, particularly slice. Code has to be updated to work with 64 bit class layout sizes.
55b6804 to
9a67114
Compare
|
I checked the presto-bytecode dependency and it looks like it only gets included via a test dependency. If we can split the bytecode target via the regular and test targets for presto-client, we can still bump presto-bytecode full tree
$ ./mvnw dependency:tree -pl presto-client
...
[INFO] --- dependency:3.8.1:tree (default-cli) @ presto-client ---
[INFO] com.facebook.presto:presto-client:jar:0.297-SNAPSHOT
[INFO] +- com.facebook.presto:presto-spi:jar:0.297-SNAPSHOT:compile
[INFO] +- com.facebook.presto:presto-common:jar:0.297-SNAPSHOT:compile
[INFO] +- com.google.errorprone:error_prone_annotations:jar:2.37.0:compile (optional)
[INFO] +- jakarta.annotation:jakarta.annotation-api:jar:2.1.1:compile (optional)
[INFO] +- com.fasterxml.jackson.core:jackson-annotations:jar:2.15.4:compile
[INFO] +- com.fasterxml.jackson.core:jackson-core:jar:2.15.4:compile
[INFO] +- com.fasterxml.jackson.core:jackson-databind:jar:2.15.4:compile
[INFO] +- com.facebook.airlift:json:jar:0.227:compile
[INFO] | +- com.google.inject:guice:jar:6.0.0:compile
[INFO] | | \- aopalliance:aopalliance:jar:1.0:compile
[INFO] | +- com.fasterxml.jackson.datatype:jackson-datatype-jdk8:jar:2.15.4:compile
[INFO] | +- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.15.4:compile
[INFO] | +- com.fasterxml.jackson.datatype:jackson-datatype-guava:jar:2.15.4:compile
[INFO] | +- com.fasterxml.jackson.datatype:jackson-datatype-joda:jar:2.15.4:compile
[INFO] | +- com.fasterxml.jackson.module:jackson-module-parameter-names:jar:2.15.4:compile
[INFO] | +- com.fasterxml.jackson.dataformat:jackson-dataformat-smile:jar:2.15.4:compile
[INFO] | \- jakarta.inject:jakarta.inject-api:jar:2.0.1:compile
[INFO] +- com.facebook.airlift:security:jar:0.227:compile
[INFO] | \- com.facebook.airlift:log:jar:0.227:compile
[INFO] +- com.facebook.airlift.drift:drift-api:jar:0.227:compile
[INFO] +- com.facebook.airlift:units:jar:0.227:compile
[INFO] +- com.google.guava:guava:jar:32.1.0-jre:compile
[INFO] | +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] | +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] | +- org.checkerframework:checker-qual:jar:3.52.0:compile
[INFO] | \- com.google.j2objc:j2objc-annotations:jar:3.0.0:compile
[INFO] +- com.google.auth:google-auth-library-oauth2-http:jar:1.39.1:compile
[INFO] | +- com.google.auto.value:auto-value-annotations:jar:1.11.0:compile
[INFO] | +- com.google.auth:google-auth-library-credentials:jar:1.39.1:compile
[INFO] | +- com.google.http-client:google-http-client:jar:1.47.0:compile
[INFO] | | +- org.apache.httpcomponents:httpclient:jar:4.5.14:compile
[INFO] | | | \- commons-codec:commons-codec:jar:1.17.2:compile
[INFO] | | +- org.apache.httpcomponents:httpcore:jar:4.4.16:compile
[INFO] | | +- io.grpc:grpc-context:jar:1.75.0:compile
[INFO] | | | \- io.grpc:grpc-api:jar:1.75.0:runtime
[INFO] | | +- io.opencensus:opencensus-api:jar:0.31.1:compile
[INFO] | | \- io.opencensus:opencensus-contrib-http-util:jar:0.31.1:compile
[INFO] | +- com.google.http-client:google-http-client-gson:jar:1.47.0:compile
[INFO] | +- com.google.api:api-common:jar:2.53.0:compile
[INFO] | | \- javax.annotation:javax.annotation-api:jar:1.3.1:compile
[INFO] | \- com.google.code.gson:gson:jar:2.12.1:compile
[INFO] +- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] | \- com.squareup.okio:okio:jar:3.6.0:compile
[INFO] +- com.squareup.okhttp3:okhttp-urlconnection:jar:4.12.0:compile
[INFO] +- com.squareup.okio:okio-jvm:jar:3.9.1:compile
[INFO] | \- org.jetbrains.kotlin:kotlin-stdlib:jar:1.9.25:compile
[INFO] | \- org.jetbrains:annotations:jar:26.0.2:compile
[INFO] +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] +- org.jetbrains.kotlin:kotlin-stdlib-jdk8:jar:1.9.25:compile
[INFO] | \- org.jetbrains.kotlin:kotlin-stdlib-jdk7:jar:1.9.25:compile
[INFO] +- net.jodah:failsafe:jar:2.4.4:compile
[INFO] +- com.facebook.airlift:concurrent:jar:0.227:test
[INFO] +- com.facebook.presto:presto-testng-services:jar:0.297-SNAPSHOT:test
[INFO] +- org.assertj:assertj-core:jar:3.8.0:test
[INFO] +- org.testng:testng:jar:7.5:test
[INFO] | +- org.slf4j:slf4j-api:jar:2.0.16:test
[INFO] | +- com.beust:jcommander:jar:1.78:test
[INFO] | \- org.webjars:jquery:jar:3.5.1:test
[INFO] +- javax.inject:javax.inject:jar:1:compile
[INFO] +- com.facebook.airlift.drift:drift-protocol:jar:0.227:test
[INFO] +- com.facebook.airlift.drift:drift-codec:jar:0.227:test
[INFO] | +- io.airlift:parameternames:jar:1.3:test
[INFO] | \- com.facebook.airlift:bytecode:jar:1.3:test
[INFO] | +- org.ow2.asm:asm:jar:9.7.1:test
[INFO] | +- org.ow2.asm:asm-tree:jar:9.7.1:test
[INFO] | +- org.ow2.asm:asm-util:jar:9.7.1:test
[INFO] | \- org.ow2.asm:asm-analysis:jar:9.7.1:test
[INFO] +- com.facebook.airlift.drift:drift-codec-utils:jar:0.227:test
[INFO] | \- joda-time:joda-time:jar:2.14.0:test
[INFO] \- com.squareup.okhttp3:mockwebserver:jar:4.12.0:test
[INFO] \- junit:junit:jar:4.13.2:test
[INFO] \- org.hamcrest:hamcrest-core:jar:1.3:test
...
The PR in the current state looks good to me though. |
ZacBlanco
left a comment
There was a problem hiding this comment.
I would prefer to get one more stamp from the more active maintainers before merging this. LGTM from me though
|
Back to in-progress due to: #27423 |
|
@mblanco-denodo — Could you please confirm when you plan to merge this change? I have an OSS Presto PR that depends on the Airlift 0.229 release. I can proceed with my Airlift changes to OSS Presto only after this change is merged. |
Description:
To update jol-core some other elements need to be upgraded, particularly slice. Code has to be updated to work with 64 bit class layout sizes.
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: