peter-toth commented on code in PR #869:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/869#discussion_r4081120351
##########
docs/architecture.md:
##########
@@ -155,7 +155,7 @@ stateDiagram-v2
ResourceReleased --> [*]
%% Place TerminatedWithoutReleaseResources further to avoid overlap
- Failures --> TerminatedWithoutReleaseResources : Retain Policy
+ Failures --> TerminatedWithoutReleaseResources : Retain Policy, No Restart
Succeeded --> TerminatedWithoutReleaseResources
Review Comment:
**Finding 5.** This edge has the same precondition as the one above it, and
now that the one above spells it out, the bare edge reads as unconditional.
Its condition is in fact the narrower of the two. I printed
`retainReleaseResourceForPolicyAndState` for every stopping state and every
policy:
```
Succeeded -> Always=true OnFailure=false Never=false
Failed -> Always=true OnFailure=true Never=false
SchedulingFailure -> Always=true OnFailure=true Never=false
DriverEvicted -> Always=true OnFailure=true Never=false
DriverStartTimedOut -> Always=true OnFailure=true Never=false
DriverReadyTimedOut -> Always=true OnFailure=true Never=false
ExecutorsStartTimedOut -> Always=true OnFailure=true Never=false
```
So `Failures --> TerminatedWithoutReleaseResources` is reached under
`Always` or `OnFailure`, which is what "Retain Policy" says, while `Succeeded
-->` needs `Always` specifically. `OnFailure` never retains a succeeded
application:
```suggestion
Succeeded --> TerminatedWithoutReleaseResources : Retain Always, No
Restart
```
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/status/ApplicationStatus.java:
##########
@@ -167,7 +167,7 @@ public ApplicationStatus terminateOrRestart(
if (exceededLimit) {
if (stateMessageOverride != null && !stateMessageOverride.isEmpty()) {
- stateMessage += stateMessageOverride;
+ stateMessage += " " + stateMessageOverride;
Review Comment:
**Finding 6.** Nothing in this module's own suite notices if the space goes
away.
I removed it and ran `:spark-operator-api:test`: 52 tests, all green. The
only test that reaches this concatenation with a non-null override is
`ApplicationStatusTest.testTerminateOrRestartWithRetry` at `:159-165`, and its
assertion is `contains(messageOverride)`, which passes either way. The space is
pinned only by `AppCleanUpStepTest` over in `:spark-operator`.
Tightening that one assertion pins it where the code lives. I applied this
and measured both arms: it passes at this head and fails with the space removed.
```java
assertEquals(
String.format(Constants.EXCEED_MAX_RETRY_ATTEMPT_MESSAGE, 1L) + " "
+ messageOverride,
maxRestartExceededReleaseResource.getCurrentState().getMessage());
```
replacing `ApplicationStatusTest.java:164-165`, plus an `import
org.apache.spark.k8s.operator.Constants;`.
--
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]
