peter-toth commented on code in PR #327:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/327#discussion_r2366142941
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/KubernetesMetricsInterceptor.java:
##########
@@ -90,25 +88,62 @@ public KubernetesMetricsInterceptor() {
}
/**
- * Intercepts an HTTP request and updates Kubernetes client metrics.
+ * Called before a request to allow for the manipulation of the request
*
- * @param chain The Interceptor.Chain for the current request.
- * @return The Response from the intercepted request.
- * @throws IOException if an I/O error occurs during the request.
+ * @param builder used to modify the request
+ * @param request the current request
*/
- @NotNull
@Override
- public Response intercept(@NotNull Chain chain) throws IOException {
- Request request = chain.request();
+ public void before(BasicBuilder builder, HttpRequest request, RequestTags
tags) {
updateRequestMetrics(request);
- Response response = null;
- final long startTime = System.nanoTime();
- try {
- response = chain.proceed(request);
- return response;
- } finally {
- updateResponseMetrics(response, startTime);
- }
+ }
+
+ /**
+ * Called after a non-WebSocket HTTP response is received. The body might or
might not be already
+ * consumed.
+ *
+ * <p>Should be used to analyze response codes and headers, original
response shouldn't be
+ * altered.
+ *
+ * @param request the original request sent to the server.
+ * @param response the response received from the server.
+ */
+ @Override
+ public void after(
+ HttpRequest request,
+ HttpResponse<?> response,
+ AsyncBody.Consumer<List<ByteBuffer>> consumer) {
+ updateResponseMetrics(response, System.nanoTime());
+ }
+
+ /**
+ * Called after a websocket failure or by default from a normal request.
+ *
+ * <p>Failure is determined by HTTP status code and will be invoked in
addition to {@link
+ * Interceptor#after(HttpRequest, HttpResponse, AsyncBody.Consumer)}
+ *
+ * @param builder used to modify the request
+ * @param response the failed response
+ * @return true if the builder should be used to execute a new request
+ */
+ @Override
+ public CompletableFuture<Boolean> afterFailure(
+ BasicBuilder builder, HttpResponse<?> response, RequestTags tags) {
+ updateResponseMetrics(null, System.nanoTime());
+ return CompletableFuture.completedFuture(false);
Review Comment:
Just a nit that now that the new `Interceptor` has sepated success
(`after()`) and failure (`afterFailure()`, `afterConnectionFailure()`) handling
methods, it might make sense to split our `updateResponseMetrics()` method as
well. It seems a bit weird that we explicitely pass `null` and
`System.nanoTime()`, which is then never used inside `updateResponseMetrics()`.
--
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]
