yuqi1129 commented on code in PR #12635:
URL: https://github.com/apache/gravitino/pull/12635#discussion_r3861164298
##########
trino-connector/trino-connector-473-478/build.gradle.kts:
##########
@@ -52,11 +52,11 @@ dependencies {
implementation(project(":catalogs:catalog-common"))
implementation(project(":clients:client-java-runtime", configuration =
"shadow"))
implementation(libs.airlift.json)
- implementation(libs.bundles.log4j)
implementation(libs.commons.collections4)
implementation(libs.commons.lang3)
implementation("io.trino:trino-jdbc:$trinoVersion")
runtimeOnly("io.opentelemetry.semconv:opentelemetry-semconv:$otelSemconvVersion")
+ compileOnly(libs.airlift.log)
Review Comment:
Dropping `libs.bundles.log4j` also drops `log4j-slf4j2-impl`, which was the
only SLF4J provider in the plugin directory. `slf4j-api` still ships
transitively through `catalogs:catalog-common`, and the bundled Gravitino code
still logs through SLF4J (`catalog-common` `PropertyConverter`, `common`
`Config`, and everything inside `client-java-runtime`, which relocates
Guava/Jackson/commons but not `org.slf4j`).
With no provider on the plugin classpath those log lines are silently
discarded and Trino's log shows `SLF4J: No SLF4J providers were found`, so the
PR's goal of "Trino admins see Gravitino connector logs" is only half met.
Shipping `slf4j-jdk14` would route them into JUL and therefore into
`var/log/server.log` alongside the airlift output.
##########
trino-connector/trino-connector-440-445/build.gradle.kts:
##########
@@ -52,11 +52,11 @@ dependencies {
implementation(project(":catalogs:catalog-common"))
implementation(project(":clients:client-java-runtime", configuration =
"shadow"))
implementation(libs.airlift.json)
- implementation(libs.bundles.log4j)
implementation(libs.commons.collections4)
implementation(libs.commons.lang3)
implementation("io.trino:trino-jdbc:$trinoVersion")
runtimeOnly("io.opentelemetry.semconv:opentelemetry-semconv:$otelSemconvVersion")
+ compileOnly(libs.airlift.log)
Review Comment:
`compileOnly` will break plugin loading on a real Trino server (same in the
`446-451`, `452-468`, `469-472` and `473-478` modules).
`io.airlift:log` declared `compileOnly` is not on
`configurations.runtimeClasspath`, and that's exactly what
`copyRuntimeLibs`/`copyLibs` ships into
`distribution/gravitino-trino-connector-*/`. Trino loads the plugin with
`PluginClassLoader`, whose parent is the platform class loader and whose SPI
parent-first prefixes are only `io.trino.spi.`,
`com.fasterxml.jackson.annotation.`, `io.airlift.slice.`, `org.openjdk.jol.`,
`io.opentelemetry.api.`, `io.opentelemetry.context.` — `io.airlift.log.` is not
among them. No other runtime dependency supplies the package
(`io.airlift:json:237` pulls slice/configuration/jackson only; `trino-jdbc`
shades nothing under `io/airlift/log`). So the first touch of
`GravitinoPlugin`/`GravitinoConnectorFactory` should throw
`NoClassDefFoundError: io/airlift/log/Logger`.
Unit tests don't catch this because `trino-testing` puts `io.airlift:log` on
the *test* classpath.
Suggest `implementation` (or `runtimeOnly`) so the jar is bundled — Trino's
own plugin directories each ship `log-<ver>.jar`. The `<scope>provided</scope>`
snippet now recommended in `docs/trino-connector/development.md` needs the same
fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
