ykisana commented on PR #918: URL: https://github.com/apache/spark-kubernetes-operator/pull/918#issuecomment-5966126469
> Thank you for updating the PR, @ykisana. The delegating `TimedConsumer` fixes the near-zero values of regular requests, and the linters, `javadoc` and the tests pass locally. > > I left inline comments. Here is the summary. > > 1. **Re-sent request**: fabric8 re-sends a request after `afterFailure(...)` with the same consumer. So the latency of the re-sent response is measured from the start of the first attempt, and the first exchange is counted twice, e.g. `401` -> token refresh -> `200`. A `5xx` or `429` retry gets a fresh consumer instead. > > 2. **`401` latency**: `after(...)` of a failed response runs only after the synchronous `afterFailure(...)` work. So a `401` sample can include the token refresh, e.g. a kubeconfig exec credential plugin. > > 3. **Rebase**: This PR conflicts with `main` in `docs/migration_guide.md` because of [SPARK-59935](https://issues.apache.org/jira/browse/SPARK-59935). [SPARK-59935](https://issues.apache.org/jira/browse/SPARK-59935) is also needed for a correct Prometheus `_sum` of this histogram. > > 4. **Flaky test**: `testResponseLatencyOfWebSocketUpgrade` awaits only `http.response`, so it can fail with an NPE at `http.response.101`. > > 5. **Javadoc and docs**: The re-send sentence of `after(...)` does not hold for a WebSocket upgrade. `after(...)` gets the outermost consumer of the chain, not necessarily the one returned by `consumer(...)`. The latency ends when the response headers arrive. 1.0 recorded less than a microsecond. > > 6. **Test coverage**: No test covers the delegation in `TimedConsumer#unwrap`, which `HttpLoggingInterceptor` needs at the `TRACE` level. > > 7. **PR description**: Please add `Generated-by: Claude Opus 5.5` as the PR template asks. Please also update `How was this patch tested?`, which still describes a 500 ms delay while the test uses 50 ms and does not mention the two other tests, and `Does this PR introduce any user-facing change?`, which does not mention that the count no longer includes WebSocket upgrades. > > 8. **Nits**: an unneeded `await()`, the nullable `Long` parameter, the anonymous `403` re-send interceptor, and the metric lookups in the tests. Addressed these, thanks for looking into it so much. Opus was used to address all these, updated the PR desc. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
