milamberspace commented on PR #6742: URL: https://github.com/apache/jmeter/pull/6742#issuecomment-5342326409
### Code review Found 3 issues, all minor relative to the rest of the PR: 1. `setupRequest` runs before `result.sampleStart()` in `HTTPHC5Impl.sample()`. If it throws (e.g. an invalid `Content-Encoding` charset, or an unresolvable upload file), the `catch` block calls `result.sampleEnd()` with `startTime` still at its zero default, so the error sample reports an elapsed time computed from the epoch instead of the real (small) one. `HTTPHC4Impl` avoids this by calling `sampleStart()` before building the request. https://github.com/apache/jmeter/blob/60c9066865e5706ca02c0b58c059bf8b0c52918e/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPHC5Impl.java#L517-L521 2. Stale Javadoc: `HttpClientKey` still says "used as the key to the ThreadLocal map of HttpClient instances", but the field itself is now a synchronized `WeakHashMap<JMeterContext, ...>`, not a `ThreadLocal`. https://github.com/apache/jmeter/blob/60c9066865e5706ca02c0b58c059bf8b0c52918e/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java#L941-L945 3. `HTTPMessageSizes`'s class Javadoc says it's "shared by the sampler implementations so that all of them report the same number of sent bytes" — in practice only `HTTPHC5Impl` uses it; `HTTPHC4Impl` still derives sent bytes from its own connection metrics, so the claimed cross-implementation consistency doesn't actually hold. https://github.com/apache/jmeter/blob/60c9066865e5706ca02c0b58c059bf8b0c52918e/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPMessageSizes.java#L21-L26 -- 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]
