peter-toth commented on code in PR #831:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/831#discussion_r4028038199
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/EventUtils.java:
##########
@@ -53,8 +58,28 @@ public final class EventUtils {
/** Maximum number of links followed when looking for the innermost cause of
a failure. */
private static final int MAX_CAUSE_DEPTH = 10;
+ /** States that are not failures but still deserve the attention of users. */
Review Comment:
**Finding 2.** "deserve the attention of users" does not say why
`InitializedBelowThresholdExecutors` is not in this set. Both states mean the
app is below its executor threshold, and the startup one is below the *minimum*
required rather than merely under capacity, so on the wording alone it reads
like the stronger candidate of the two.
The rule that actually separates them is elsewhere.
`AppDriverTimeoutObserver.java:78` escalates
`InitializedBelowThresholdExecutors` to `ExecutorsStartTimedOut`, which is a
failure and therefore already a `Warning`, so the startup case reports itself
when it stops being transient. `RunningWithBelowThresholdExecutors` appears in
no timeout branch: it persists silently, so nothing else would ever tell the
user. `RunningWithPartialCapacity` is a third case, documented on the enum as a
tolerable capacity level rather than a degradation.
Worth one sentence here, because this is the comment someone will read when
they add a state:
```java
/**
* States that are not failures but still deserve the attention of users,
because nothing else
* reports them: a state that escalates to a failure on its own, such as
* {@code InitializedBelowThresholdExecutors} timing out into {@code
ExecutorsStartTimedOut},
* warns through that failure instead and stays normal here.
*/
```
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/utils/EventUtilsTest.java:
##########
@@ -60,6 +63,44 @@ void warnKeysTheEventOnReasonSoRepeatsAggregate() {
assertThat(event.key()).contains(EventUtils.REASON_STATUS_UPDATE_FAILED);
}
+ @Test
+ void recordPublishesTheGivenType() {
+ EventUtils.record(recorder, EventType.NORMAL, "DriverRequested", "driver
requested");
+
+ ArgumentCaptor<EventRecord> captor =
ArgumentCaptor.forClass(EventRecord.class);
+ verify(recorder).record(captor.capture());
+ EventRecord event = captor.getValue();
+ assertThat(event.type()).isEqualTo(EventType.NORMAL);
+ assertThat(event.reason()).isEqualTo("DriverRequested");
+ assertThat(event.message()).isEqualTo("driver requested");
+ assertThat(event.key()).contains("DriverRequested");
+ }
+
+ @Test
+ void eventTypeOfApplicationStates() {
+ for (ApplicationStateSummary summary : ApplicationStateSummary.values()) {
+ EventType expected =
Review Comment:
**Finding 1.** `expected` is the same expression as
`EventUtils.eventTypeOf`, so this loop asserts that the method agrees with
itself. It pins the two `NON_FAILURE_WARNING_STATES` members, which is worth
having, but it delegates the other half of the classification to `isFailure()`
and therefore follows that set wherever it goes.
That matters because the classification is published as a contract: the
eight `Warning` reasons are listed in the PR description and in
`docs/configuration.md:64-66`. I checked what the test would catch by dropping
`DriverEvicted` from `ApplicationStateSummary.failures`:
```
$ ./gradlew :spark-operator:test --tests "...EventUtilsTest"
tests="13" skipped="0" failures="0" errors="0"
```
Green, while `DriverEvicted` would now publish as `Normal` and the docs
table would be wrong.
An explicit set makes the loop pin the documented table instead:
```java
@Test
void eventTypeOfApplicationStates() {
Set<ApplicationStateSummary> expectedWarnings =
Set.of(
ApplicationStateSummary.SchedulingFailure,
ApplicationStateSummary.Failed,
ApplicationStateSummary.DriverEvicted,
ApplicationStateSummary.DriverStartTimedOut,
ApplicationStateSummary.DriverReadyTimedOut,
ApplicationStateSummary.ExecutorsStartTimedOut,
ApplicationStateSummary.RunningWithBelowThresholdExecutors,
ApplicationStateSummary.TerminatedWithoutReleaseResources);
for (ApplicationStateSummary summary : ApplicationStateSummary.values())
{
EventType expected =
expectedWarnings.contains(summary) ? EventType.WARNING :
EventType.NORMAL;
assertThat(EventUtils.eventTypeOf(summary)).as(summary.name()).isEqualTo(expected);
}
}
```
That also makes the two trailing `RunningWithPartialCapacity` /
`InitializedBelowThresholdExecutors` assertions redundant, since the loop now
covers them by exclusion. The same shape applies to `eventTypeOfClusterStates`
at line 99, where `Set.of(SchedulingFailure, Failed)` is the whole table.
--
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]
