gnodet-bot commented on code in PR #2236:
URL: https://github.com/apache/maven-resolver/pull/2236#discussion_r4388651237
##########
maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java:
##########
@@ -822,28 +856,50 @@ public boolean retryRequest(HttpResponse response, int
executionCount, HttpConte
*/
private Long retryInterval(HttpResponse httpResponse, int
executionCount, HttpContext httpContext) {
Long result = null;
+ String intervalReason = "";
Header header =
httpResponse.getFirstHeader(HttpHeaders.RETRY_AFTER);
if (header != null && header.getValue() != null) {
String headerValue = header.getValue();
if (headerValue.contains(":")) { // is date when to retry
Date when = DateUtils.parseDate(headerValue); //
presumably future
if (when != null) {
result = Math.max(when.getTime() -
System.currentTimeMillis(), 0L);
+ intervalReason = "Retry-After header with date value "
+ headerValue;
+ } else {
+ LOGGER.warn(
+ "Invalid Retry-After header value {} for
request to {}",
+ headerValue,
+
httpContext.getAttribute(HttpCoreContext.HTTP_TARGET_HOST));
}
} else {
try {
result = Long.parseLong(headerValue) * 1000L; // is in
seconds
+ intervalReason = "Retry-After header with seconds
value " + headerValue;
} catch (NumberFormatException e) {
// fall through
+ LOGGER.warn(
+ "Invalid Retry-After header value {} for
request to {}",
+ headerValue,
+
httpContext.getAttribute(HttpCoreContext.HTTP_TARGET_HOST));
}
}
}
if (result == null) {
result = executionCount * this.retryInterval;
+ intervalReason =
+ "retryInterval " + this.retryInterval + " ms
multiplied by execution count " + executionCount;
}
if (result > retryIntervalMax) {
- return null;
+ intervalReason += ", capped at max value";
+ result = retryIntervalMax;
}
Review Comment:
⚠️ **Behavioral change (potential compatibility break):** The old code
returned `null` when `result > retryIntervalMax`, which the retry handler
interprets as **"abort retry"** — the request would not be retried if the
computed delay exceeded the maximum. The new code instead caps the delay and
continues retrying.
This is a silent semantic change that removes the abort path entirely.
Callers that configured a `retryIntervalMax` specifically to bound retries by
delay duration will now always retry at the max interval instead of giving up.
Please:
1. Document this change clearly in the method Javadoc and/or the PR
description
2. Add a test case explicitly covering the `result > retryIntervalMax`
branch under the new semantics
3. Consider whether this change should be flagged in release notes
##########
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
Review Comment:
📝 **Incomplete Javadoc — empty `@param` descriptions:** `@param logger`,
`@param method`, and `@param uri` are documented without descriptions on both
`logImmediateRetry` (lines 45-47) and `logRetry` (lines 60-62). These are
public SPI `static` methods — all `@param` tags must have meaningful
descriptions.
Suggested additions:
- `@param logger the logger to use for logging the retry event`
- `@param method the HTTP method of the request being retried (e.g. `GET`,
`PUT`)`
- `@param uri the URI of the request being retried`
##########
maven-resolver-test-http/src/main/resources/logback.xml:
##########
@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
⚠️ **Wrong resource directory:** `logback.xml` placed in
`src/main/resources` ships inside the `maven-resolver-test-http` JAR and ends
up on the classpath of every module that depends on this artifact with
`<scope>test</scope>`. When two `logback.xml` files are on the test classpath,
Logback picks one arbitrarily — the consumer module's own configuration may be
silently overridden.
Please move this file to `src/test/resources/` — that's the correct location
for configuration that controls the test environment of this module only. Java
helper classes in `src/main/java` are intentional (they're shared), but
resource files are not.
##########
maven-resolver-test-http/pom.xml:
##########
@@ -115,9 +115,11 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <!-- used to capture log events -->
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-simple</artifactId>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <version>1.5.32</version>
Review Comment:
🔧 **Hardcoded version violates project convention:** All library versions
are centralized in the root `pom.xml` via `<dependencyManagement>` and
`<version.X>` properties. Please:
1. Add `<version.logback>1.5.32</version.logback>` to the root `pom.xml`
properties
2. Add a `<dependencyManagement>` entry for `logback-classic` using
`${version.logback}`
3. Remove the `<version>` tag from this child module's declaration
Note: also check if `maven-resolver-transport-jetty` or other modules
already pull logback transitively — if so, the BOM entry should go in the root
POM's `<dependencyManagement>` to govern all usages consistently.
--
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]
