gnodet commented on PR #1957: URL: https://github.com/apache/maven-resolver/pull/1957#issuecomment-4957762362
Thanks for the thorough review! Addressing the observations: 1. **Batch trace assumption**: Agreed — in practice all requests in a batch share the same trace context since they're constructed within a single operation. Adding an assertion or enforcement would be defensive but could also be confusing since there's no documented contract for this. Happy to add a comment clarifying the assumption if desired. 2. **Null-check ordering**: Correct, `requireNonNull(request)` now precedes `validateSession(session)` since we need the request to access the trace. The behavioral change is negligible (different exception when both are null). 3. **Shutdown on re-entry**: Right — the outermost call owns the lifecycle contract, and re-entrant calls within an in-flight operation should proceed regardless. This is consistent with the general principle that shutdown is cooperative, not preemptive. Regarding cross-thread testing: as you noted, the mechanism is purely trace-chain-based with no thread affinity, so the single-threaded tests are sufficient. The `BfDependencyCollector.SmartExecutor` propagates the `RequestTrace` to pool threads, which is what makes the trace-based approach work across thread boundaries. A companion PR for Maven will follow to remove the consumer-side dependency filtering workarounds that are no longer needed with this 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]
