dongjoon-hyun commented on code in PR #860:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/860#discussion_r4065984384
##########
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:
Good catch. `SslHandshakeTimeoutException` does extend
`SSLHandshakeException`, so the check was rejecting it.
Rather than special-casing the Netty type, I keyed the permanent check on
the trust failures themselves, which is the set fabric8 uses in
`StandardHttpClient.isTerminalTlsTrustFailure`: `CertificateException`,
`CertPathValidatorException`, `CertPathBuilderException` and
`SSLPeerUnverifiedException`. A rejected certificate nests one of those
(`sun.security.validator.ValidatorException` extends `CertificateException`,
`SunCertPathBuilderException` extends `CertPathBuilderException`), while a
handshake that merely timed out does not, so it stays retryable. This also
stops the check from depending on which client reported the handshake.
Verified against the pinned client: a handshake timeout is now transient, a
certificate rejection and a peer-unverified failure stay permanent, and a bare
`SSLException` from a mid-connection reset is transient.
Added `classifiesHandshakeTimeoutAsTransient`, and
`doesNotClassifyCertificateFailureAsTransient` now nests a
`CertificateException` under the handshake so the rejection case stays covered.
##########
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:
Agreed, and thank you for the warning about `InterruptedIOException` —
`SocketTimeoutException` extends it, so excluding the whole class would have
made plain read timeouts permanent.
The check now matches `InterruptedException` in the cause chain, which is
what `waitForResult()` puts under the `InterruptedIOException` and which a read
timeout never carries.
Verified by interrupting a call in flight against the pinned client: code
`-1`, cause chain `InterruptedIOException <- InterruptedException`, interrupt
flag still set, now classified permanent — while `SocketTimeoutException` stays
transient.
Added `doesNotClassifyInterruptionAsTransient`.
--
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]
