gnodet-bot commented on code in PR #2236:
URL: https://github.com/apache/maven-resolver/pull/2236#discussion_r4388169836
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/transport/http/HttpTransporter.java:
##########
@@ -36,4 +39,61 @@ default int classify(Throwable error) {
}
return ERROR_OTHER;
}
+
+ /**
+ *
+ * @param logger
+ * @param method
+ * @param uri
+ * @param requestAttempt the 0-based counter of the previous request
attempt (0 for the first attempt, 1 for the first retry, etc.)
+ * @param maxRequestAttempt the maximum number of request attempts (this
is including this original request)
+ * @param exception the exception that caused the immediate retry
+ */
+ static void logImmediateRetry(
+ Logger logger, String method, URI uri, int requestAttempt, int
retryCount, Throwable exception) {
+ String reason = String.format("%s: %s",
exception.getClass().getName(), exception.getMessage());
+ logRetry(logger, method, uri, requestAttempt, retryCount, reason, 0L,
null);
Review Comment:
⚠️ **`@param` name mismatch — will fail Checkstyle CI.** The Javadoc
declares `@param maxRequestAttempt` (line 49) but the actual parameter is named
`retryCount`. Checkstyle's `@param` validation enforces tag names match
parameter names — this will break the build. Rename `retryCount` to
`maxRequestAttempt` for consistency with `logRetry()`, and update the
forwarding call:
```suggestion
static void logImmediateRetry(
Logger logger, String method, URI uri, int requestAttempt, int
maxRequestAttempt, Throwable exception) {
String reason = String.format("%s: %s",
exception.getClass().getName(), exception.getMessage());
logRetry(logger, method, uri, requestAttempt, maxRequestAttempt,
reason, 0L, null);
}
```
##########
maven-resolver-test-http/src/main/java/org/eclipse/aether/internal/test/util/http/RetryLogCapture.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.eclipse.aether.internal.test.util.http;
+
+import java.util.List;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+final class RetryLogCapture implements AutoCloseable {
+ private final Logger logger;
+
+ private final ListAppender<ILoggingEvent> appender = new ListAppender<>();
+
+ RetryLogCapture(Class<?> transporterClass) {
+ logger = (Logger) LoggerFactory.getLogger(transporterClass);
+ appender.start();
+ logger.addAppender(appender);
+ }
+
+ List<String> messages() {
+ return appender.list.stream()
+ .filter(event ->
event.getFormattedMessage().contains("Retrying"))
+ .peek(event -> assertEquals(Level.WARN, event.getLevel()))
+ .map(ILoggingEvent::getFormattedMessage)
+ .toList();
+ }
Review Comment:
💡 **Side-effecting `peek()` is a code smell.** Using `assertEquals` inside
`peek()` works today because `toList()` always fully consumes the stream — but
it conflates filtering with assertion and will silently stop asserting if any
caller ever wraps this in a short-circuiting operation. Prefer collecting
first, then asserting separately:
```suggestion
List<String> messages() {
List<ILoggingEvent> retryEvents = appender.list.stream()
.filter(event ->
event.getFormattedMessage().contains("Retrying"))
.toList();
retryEvents.forEach(event -> assertEquals(Level.WARN,
event.getLevel()));
return retryEvents.stream()
.map(ILoggingEvent::getFormattedMessage)
.toList();
}
```
--
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]
