peter-toth commented on code in PR #916: URL: https://github.com/apache/spark-kubernetes-operator/pull/916#discussion_r4158630769
########## docs/configuration.md: ########## @@ -223,6 +229,10 @@ via [Codahale JVM Metrics](https://javadoc.io/doc/com.codahale.metrics/metrics-j | kubernetes.client.`ResourceName`.`Method` | Meter | Tracking the rates of HTTP request for a combination of one Kubernetes resource and one http method | | kubernetes.client.`NamespaceName`.`ResourceName`.`Method` | Meter | Tracking the rates of HTTP request for a combination of one namespace-scoped Kubernetes resource and one http method | +A request which gets no response is counted in `kubernetes.client.http.response.failed` only when +the client retries it. So a request which the client does not retry, i.e. a Kubernetes event write +or the update of the Kueue `PodsReady` condition, is not counted there when it gets no response. Review Comment: **Finding 1.** This note doesn't match the code in two places. I checked both with a throwaway test against the mock server. - **Name.** `KubernetesMetricsInterceptor` registers the meter as `failed`, and `MetricsSystem.registerSource` adds the source name in front. So the metric is `kubernetes.client.failed`. There is no `kubernetes.client.http.response.failed`. - **When it counts.** fabric8 calls `afterConnectionFailure` for every attempt that gets no response while the request still has a retry left. It does this even when `StandardHttpClient.shouldRetry` then gives up. A request timeout is that case. vertx fails it with `NoStackTraceTimeoutException`, which is not an `IOException`, so fabric8 doesn't retry it. I sent a GET through the operator client with `requestTimeout=1000`, and the server delayed its answer by 3s. It failed after 1006 ms, sent one request and still moved `failed` from 1 to 2. So "only when the client retries it" doesn't hold for the main client. The point about event writes and `PodsReady` is still right. The same timeout through `withoutRetries` left `failed` at 2. ```suggestion A request which gets no response is counted in `kubernetes.client.failed` while the client still has a retry left for it, even if it does not retry it, e.g. on a request timeout. So a request of a client which never retries, i.e. a Kubernetes event write or the recording of the Kueue `PodsReady` condition, is not counted there when it gets no response. ``` The `withoutRetries` javadoc (`KubernetesClientFactory.java:79`) and the test comment at `KubernetesClientFactoryTest.java:68` give the same "only before a retry" reason. What they conclude about the derived client is right, so only the reason needs the same fix. The table rows above are older and also wrong. `failed` also counts every non-2xx response through `afterFailure`: a 404 GET moved it from 0 to 1. The `1xx`-`5xx` rows are really `kubernetes.client.1xx` and so on. I filed [SPARK-59927](https://issues.apache.org/jira/browse/SPARK-59927) for those. -- 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]
