viirya commented on code in PR #860:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/860#discussion_r4066028002
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -297,20 +315,53 @@ public static boolean isFirstAttempt(Context<?> context) {
}
/**
- * Whether the given failure is transport level rather than a decision by
the API server.
+ * Whether the given failure is expected to clear without anyone acting on
it, so that a caller
+ * may wait it out rather than report it. A broken connection and an
overloaded or proxied server
+ * qualify; a rejection by a reachable API server and a client side
rejection do not.
*
* @param e The failure to classify.
- * @return True if the request did not reach a healthy API server, false
otherwise.
+ * @return True if the failure is expected to clear on its own, false
otherwise.
*/
public static boolean isTransientError(KubernetesClientException e) {
- // code 0 is fabric8's sentinel for network-level failures (timeouts,
connection resets, etc.)
return switch (e.getCode()) {
- case 0, HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY,
- HTTP_UNAVAILABLE, HTTP_GATEWAY_TIMEOUT -> true;
+ case NO_RESPONSE_CODE -> brokeOnTheWayToTheApiServer(e);
+ case HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY, HTTP_UNAVAILABLE,
HTTP_GATEWAY_TIMEOUT -> true;
default -> false;
};
}
+ /**
+ * Whether the given failure left the request unanswered, so that asking
again may yet work. It
+ * is defined as the complement of the two status-less failures that
repeating cannot change: a
+ * rejection the client raised before sending anything, which carries no
cause at all, and an
+ * answer that arrived but could not be used, which carries the parsing or
certificate failure
+ * that rejected it. Anything else that carries a cause counts as unanswered.
+ *
+ * <p>Naming what cannot work, rather than what can, keeps this from
tracking the exception types
+ * of whichever HTTP client is plugged in. A connection the peer closes
mid-response is the case
+ * that matters: the client in use reports it with a type of its own which
is not even an {@link
+ * java.io.IOException}, so any list of recognized connection failures would
silently miss it.
+ *
+ * @param e The failure to inspect.
+ * @return True if the request went unanswered, false otherwise.
+ */
+ private static boolean brokeOnTheWayToTheApiServer(KubernetesClientException
e) {
+ // Bounded walk: a cyclic cause chain must not hang the reconciler. The
marker is nested, since
+ // fabric8 wraps the failure and its own HTTP client wraps it again.
+ Throwable cause = e.getCause();
+ for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) {
+ // A handshake that failed over a certificate, a host name or a trust
store is the one
+ // answer-less failure that repeating cannot fix, so it is grouped with
the unusable answers.
+ if (cause instanceof JsonProcessingException
+ || cause instanceof SSLHandshakeException
+ || cause instanceof SSLPeerUnverifiedException) {
+ return false;
Review Comment:
Thanks, this addresses the issue. Checking the underlying trust failures
preserves retryability for `SslHandshakeTimeoutException` without coupling the
production code to Netty. The updated certificate-rejection test also retains
coverage for the permanent failure case. Resolved from my side.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -297,20 +315,53 @@ public static boolean isFirstAttempt(Context<?> context) {
}
/**
- * Whether the given failure is transport level rather than a decision by
the API server.
+ * Whether the given failure is expected to clear without anyone acting on
it, so that a caller
+ * may wait it out rather than report it. A broken connection and an
overloaded or proxied server
+ * qualify; a rejection by a reachable API server and a client side
rejection do not.
*
* @param e The failure to classify.
- * @return True if the request did not reach a healthy API server, false
otherwise.
+ * @return True if the failure is expected to clear on its own, false
otherwise.
*/
public static boolean isTransientError(KubernetesClientException e) {
- // code 0 is fabric8's sentinel for network-level failures (timeouts,
connection resets, etc.)
return switch (e.getCode()) {
- case 0, HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY,
- HTTP_UNAVAILABLE, HTTP_GATEWAY_TIMEOUT -> true;
+ case NO_RESPONSE_CODE -> brokeOnTheWayToTheApiServer(e);
+ case HTTP_CLIENT_TIMEOUT, HTTP_BAD_GATEWAY, HTTP_UNAVAILABLE,
HTTP_GATEWAY_TIMEOUT -> true;
default -> false;
};
}
+ /**
+ * Whether the given failure left the request unanswered, so that asking
again may yet work. It
+ * is defined as the complement of the two status-less failures that
repeating cannot change: a
+ * rejection the client raised before sending anything, which carries no
cause at all, and an
+ * answer that arrived but could not be used, which carries the parsing or
certificate failure
+ * that rejected it. Anything else that carries a cause counts as unanswered.
+ *
+ * <p>Naming what cannot work, rather than what can, keeps this from
tracking the exception types
+ * of whichever HTTP client is plugged in. A connection the peer closes
mid-response is the case
+ * that matters: the client in use reports it with a type of its own which
is not even an {@link
+ * java.io.IOException}, so any list of recognized connection failures would
silently miss it.
+ *
+ * @param e The failure to inspect.
+ * @return True if the request went unanswered, false otherwise.
+ */
+ private static boolean brokeOnTheWayToTheApiServer(KubernetesClientException
e) {
+ // Bounded walk: a cyclic cause chain must not hang the reconciler. The
marker is nested, since
+ // fabric8 wraps the failure and its own HTTP client wraps it again.
+ Throwable cause = e.getCause();
+ for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) {
+ // A handshake that failed over a certificate, a host name or a trust
store is the one
+ // answer-less failure that repeating cannot fix, so it is grouped with
the unusable answers.
+ if (cause instanceof JsonProcessingException
+ || cause instanceof SSLHandshakeException
+ || cause instanceof SSLPeerUnverifiedException) {
+ return false;
+ }
+ cause = cause.getCause();
+ }
+ return e.getCause() != null;
Review Comment:
Thanks, this addresses the issue. Matching `InterruptedException` in the
cause chain handles fabric8’s interruption wrapping while keeping
`SocketTimeoutException` retryable. The added regression test covers the
distinction we discussed. Resolved from my side.
--
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]
