peter-toth commented on code in PR #920:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/920#discussion_r4163637223
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/EventUtils.java:
##########
@@ -203,16 +210,24 @@ public static String describe(Throwable throwable) {
if (throwable == null) {
return "";
}
- StringBuilder builder = new StringBuilder(describeSingle(throwable));
+ String description = describeSingle(throwable);
Throwable rootCause = rootCauseOf(throwable);
- if (rootCause != null) {
- builder.append(", caused by: ").append(describeSingle(rootCause));
+ if (rootCause == null) {
+ return description;
}
- return builder.toString();
+ String rootCauseDescription = describeSingle(rootCause);
+ return rootCauseDescription.equals(description)
+ ? description
+ : description + ", caused by: " + rootCauseDescription;
}
private static String describeSingle(Throwable throwable) {
String message = throwable.getMessage();
+ if (throwable instanceof KubernetesClientException e
+ && e.getStatus() != null
+ && StringUtils.isNotBlank(e.getStatus().getMessage())) {
+ message = e.getStatus().getMessage();
+ }
String type = throwable.getClass().getSimpleName();
return StringUtils.isBlank(message) ? type : type + ": " + message;
}
Review Comment:
**Finding 1.** The dedupe compares `Type: message`, so it misses a wrapper
that repeats its cause's message under another type. A request timeout is that
case. `OperationSupport.waitForResult` rethrows the vertx
`NoStackTraceTimeoutException` as `new
KubernetesClientException(t.getMessage(), t)`, with no `Status`. I sent a POST
to the mock server with a 1 s request timeout and a 3 s delay, and `describe`
returned:
```
KubernetesClientException: The timeout period of 1000ms has been exceeded
while executing POST /api/v1/namespaces/ns-1/configmaps for server
localhost:50411, caused by: NoStackTraceTimeoutException: The timeout period of
1000ms has been exceeded while executing POST
/api/v1/namespaces/ns-1/configmaps for server localhost:50411
```
So the request URL shows up twice. The transient filter keeps this out of
most warnings. `ReconcileError` and `CleanupError` don't filter, though. A
timeout can also end in `SchedulingFailure` through the strict read in
`getOrCreateSecondaryResource`.
Comparing only the messages fixes it. With the change below, the same probe
gives one copy, and a 403 and a non-`Status` 503 read as before.
`EventUtilsTest` stays green.
```suggestion
String message = messageOf(throwable);
String description = describeSingle(throwable, message);
Throwable rootCause = rootCauseOf(throwable);
if (rootCause == null) {
return description;
}
String rootCauseMessage = messageOf(rootCause);
// A wrapper which repeats the message of its cause, e.g. the one
fabric8 throws for a request
// timeout, adds nothing but its type.
if (StringUtils.isNotBlank(message) && message.equals(rootCauseMessage))
{
return description;
}
return description + ", caused by: " + describeSingle(rootCause,
rootCauseMessage);
}
private static String messageOf(Throwable throwable) {
if (throwable instanceof KubernetesClientException e
&& e.getStatus() != null
&& StringUtils.isNotBlank(e.getStatus().getMessage())) {
return e.getStatus().getMessage();
}
return throwable.getMessage();
}
private static String describeSingle(Throwable throwable, String message) {
String type = throwable.getClass().getSimpleName();
return StringUtils.isBlank(message) ? type : type + ": " + message;
```
The javadoc's "unless it reads the same as the exception" would then become
"unless its message is the same". A test could pin it with `new
KubernetesClientException(msg, new TimeoutException(msg))`.
--
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]
