snazy commented on code in PR #2268:
URL: https://github.com/apache/polaris/pull/2268#discussion_r2254912628
##########
build-logic/src/main/kotlin/polaris-java.gradle.kts:
##########
@@ -107,11 +107,13 @@ testing {
dependencies {
implementation(project())
implementation(testFixtures(project()))
- runtimeOnly(
- libs.findLibrary("logback-classic").orElseThrow {
- GradleException("logback-classic not declared in
libs.versions.toml")
- }
- )
+ if (!plugins.hasPlugin("io.quarkus")) {
Review Comment:
Think this isn't necessary (see my comment below)
##########
persistence/relational-jdbc/src/test/resources/logback-test.xml:
##########
@@ -19,21 +19,14 @@
under the License.
-->
-<configuration>
+<configuration debug="false">
Review Comment:
Should the file be removed, since logback isn't there?
##########
runtime/service/build.gradle.kts:
##########
@@ -135,12 +136,6 @@ dependencies {
testImplementation("software.amazon.awssdk:kms")
testImplementation("software.amazon.awssdk:dynamodb")
- runtimeOnly(project(":polaris-relational-jdbc"))
- runtimeOnly("io.quarkus:quarkus-jdbc-postgresql") {
- exclude(group = "org.antlr", module = "antlr4-runtime")
- exclude(group = "org.scala-lang", module = "scala-library")
- exclude(group = "org.scala-lang", module = "scala-reflect")
Review Comment:
Those excludes are no longer necessary?
##########
runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java:
##########
@@ -87,19 +87,27 @@ public class IcebergExceptionMapper implements
ExceptionMapper<RuntimeException>
private static final Set<String> ACCESS_DENIED_HINTS =
Set.of("access denied", "not authorized", "forbidden");
- public IcebergExceptionMapper() {}
+ private final Logger logger;
Review Comment:
Nit: instead of having a field, have a function that yields `LOGGER` in
production code, but is overridden in tests. Should be easier for C2 to inline.
##########
build-logic/src/main/kotlin/polaris-runtime.gradle.kts:
##########
@@ -47,6 +54,23 @@ testing {
}
}
+dependencies {
+ // All Quarkus projects should use JBoss LogManager with SLF4J, instead of
Logback
+ implementation("org.jboss.slf4j:slf4j-jboss-logmanager")
+}
Review Comment:
You may want something like this to ban logback from Quarkus.
```
configurations.all { exclude(group = "ch.qos.logback", module =
"logback-classic") }
```
BTW: it would also need to be removed from the LICENSE/NOTICE
##########
persistence/relational-jdbc/src/test/resources/logback-test.xml:
##########
@@ -19,21 +19,14 @@
under the License.
-->
-<configuration>
+<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="console" class="ch.qos.logback.core.ConsoleAppender">
<encoder>
<pattern>%date{ISO8601} [%thread] %-5level %logger{36} - %msg%n</pattern>
</encoder>
</appender>
- <root level="${log.level.console:-INFO}">
+ <root level="${test.log.level:-ERROR}">
<appender-ref ref="console"/>
</root>
-
- <!--
- Prevent the 'The Agroal dependency is present but no JDBC datasources have
been defined.' build-time warning.
Review Comment:
Oh!
Now I understand why that only _sometimes_ worked
##########
plugins/spark/v3.5/spark/src/test/resources/logback-test.xml:
##########
@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="UTF-8"?>
+<?xml version="1.0" encoding="UTF-8" ?>
Review Comment:
Should the file be removed, since logback isn't there?
##########
runtime/service/build.gradle.kts:
##########
@@ -135,12 +136,6 @@ dependencies {
testImplementation("software.amazon.awssdk:kms")
testImplementation("software.amazon.awssdk:dynamodb")
- runtimeOnly(project(":polaris-relational-jdbc"))
- runtimeOnly("io.quarkus:quarkus-jdbc-postgresql") {
Review Comment:
Why do those need to be `implementation` deps now?
##########
runtime/test-common/src/main/java/org/apache/polaris/test/commons/junit/GradleDuplicateLoggingWorkaround.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.test.commons.junit;
+
+import jakarta.annotation.Priority;
+import jakarta.enterprise.event.Observes;
+import jakarta.enterprise.event.Startup;
+import jakarta.inject.Singleton;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.junit.jupiter.api.extension.BeforeAllCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * JUnit extension + Singleton CDI bean that "fixes" test logging
configuration.
+ *
+ * <p>This component removes the SLF4JBridgeHandler installed by Gradle, to
prevent duplicate
+ * logging messages in tests. Without this fix, Gradle tests using JBoss
LogManager (which is the
+ * default in Quarkus) will log each message twice: once via the
SLF4JBridgeHandler, without any
+ * formatting, and once via the JBoss LogManager (with formatting). This is
annoying because the
+ * non-formatted messages appear on the console, regardless of the log level.
+ *
+ * <p>Note: this issue has been reported to Quarkus and appears fixed, but
only the formatting part
+ * is fixed, not the duplicate messages.
+ *
+ * @see <a href="https://github.com/quarkusio/quarkus/issues/22844";>Gradle
tests (with JBoss
+ * LogManager setup) output duplicate unformatted messages</a>
+ * @see <a href="https://github.com/quarkusio/quarkus/issues/48763";>Gradle
+ * testLogging.showStandardStreams = false ignored by Quarkus tests with
JBoss LogManager</a>
+ */
+@Singleton
+public class GradleDuplicateLoggingWorkaround implements BeforeAllCallback {
+
+ private static final Logger LOGGER =
+ LoggerFactory.getLogger(GradleDuplicateLoggingWorkaround.class);
+
+ private static final AtomicBoolean DONE = new AtomicBoolean(false);
+
+ public void onStartup(@Observes @Priority(1) Startup event) {
+ // Sometimes the application is started before the test extension is
invoked,
+ // so we need to ensure the SLF4JBridgeHandler is removed at startup as
well.
+ removeGradleHandlers();
+ }
+
+ @Override
+ public void beforeAll(ExtensionContext context) {
+ removeGradleHandlers();
+ }
+
+ private static void removeGradleHandlers() {
+ if (!DONE.getAndSet(true)) {
+ var logger =
org.jboss.logmanager.LogManager.getLogManager().getLogger("");
+ for (var handler : logger.getHandlers()) {
+ if (handler.getClass().getSimpleName().equals("SLF4JBridgeHandler")) {
+ logger.removeHandler(handler);
+ }
+ }
+ LOGGER.warn("Removed SLF4JBridgeHandler to fix duplicate logging
messages in Gradle tests.");
Review Comment:
Nit: only log if a handler was removed?
--
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]
