peter-toth commented on code in PR #873:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/873#discussion_r4082443269
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/status/ApplicationStatus.java:
##########
@@ -96,17 +96,37 @@ public ApplicationStatus appendNewState(ApplicationState
state) {
* Creates an updated ApplicationStatus based on the termination or restart
logic.
*
* @param restartConfig The restart configuration for the application.
- * @param resourceRetainPolicy The resource retention policy for the
application.
+ * @param resourceRetainPolicy Ignored. The resources are expected to be
released already.
* @param stateMessageOverride An optional message to override the default
state message.
* @param trimStateTransitionHistory If true, the state transition history
will be trimmed.
* @return An updated ApplicationStatus object.
* @since 0.1.0
+ * @deprecated since 1.1.0, use {@link #terminateOrRestart(RestartConfig,
String, boolean)}
+ * instead.
*/
+ @Deprecated(since = "1.1.0")
Review Comment:
**Finding 1.** The reason for keeping this overload does not hold, and as it
stands the overload changes behaviour silently.
`spark-operator-api` is wired for publishing: `deploy.gradle:52-116` sets
`groupId = 'org.apache.spark'` and is applied from `build.gradle:187`. It has
never actually been released there. The `org/apache/spark/` listing on Maven
Central carries 188 artifacts and none of `spark-operator-api`,
`spark-operator` or `spark-submission-worker` is among them; each of their
paths 404s, and so does `repository.apache.org/content/repositories/snapshots`.
So there is no caller compiled against the 4-argument signature to keep it for.
Keeping it is not free either. A caller that passed `Always`, or `OnFailure`
on a failure, used to get `TerminatedWithoutReleaseResources` and now gets
`ResourceReleased`. Same signature, same return type, different state.
`@Deprecated` produces a compile warning that says "deprecated", not "the
second argument is ignored now", and the `@param resourceRetainPolicy Ignored.`
line only reaches someone who opens the javadoc.
Two ways out, either is fine by me:
* Remove the overload. Nothing external can break, and it leaves one method
instead of two.
* Keep it and make the signal match the change:
```suggestion
@Deprecated(since = "1.1.0", forRemoval = true)
```
`forRemoval = true` moves the warning into javac's `removal` category, which
is on by default, unlike plain `deprecation` which needs `-Xlint:deprecation`.
Then say it in the `@deprecated` text too, since that is what a caller reads:
```java
* @deprecated since 1.1.0, scheduled for removal. {@code
resourceRetainPolicy} is ignored, so a
* policy other than {@code Never} no longer terminates the
application as {@code
* TerminatedWithoutReleaseResources}. Use
* {@link #terminateOrRestart(RestartConfig, String, boolean)} instead.
```
##########
spark-operator-api/src/test/java/org/apache/spark/k8s/operator/status/ApplicationStatusTest.java:
##########
@@ -167,17 +131,22 @@ void testTerminateOrRestartWithRetry() {
maxRestartExceededReleaseResource.getCurrentState().getMessage());
assertEquals(
1L,
maxRestartExceededReleaseResource.getCurrentAttemptSummary().getAttemptInfo().getId());
+ }
- ApplicationStatus maxRestartExceededRetainResource =
- restartFailed.terminateOrRestart(
- alwaysRetryConfig, ResourceRetainPolicy.Always, messageOverride,
false);
- assertEquals(
- ApplicationStateSummary.TerminatedWithoutReleaseResources,
-
maxRestartExceededRetainResource.getCurrentState().getCurrentStateSummary());
- assertTrue(
-
maxRestartExceededRetainResource.getCurrentState().getMessage().contains(messageOverride));
- assertEquals(
- 1L,
maxRestartExceededRetainResource.getCurrentAttemptSummary().getAttemptInfo().getId());
+ @Test
+ @SuppressWarnings("deprecation")
+ void testDeprecatedTerminateOrRestartIgnoresResourceRetainPolicy() {
+ RestartConfig noRetryConfig = new RestartConfig();
+ noRetryConfig.setRestartPolicy(RestartPolicy.Never);
Review Comment:
**Finding 2.** This line confines the test to one of the two branches you
removed a guard from.
With `RestartPolicy.Never` and a `Failed` state, `attemptRestartOnState` is
false, so every iteration returns from the "no restart configured" branch at
`ApplicationStatus.java:138-148`. The second guard you removed sat in the
restart-limit-exceeded branch at `:193-202`, and nothing reaches that one
through the 4-argument method: the exceeded case in
`testTerminateOrRestartWithRetry` uses the new 3-argument overload, which has
no policy to ignore in the first place.
A second loop covers it. I applied this and measured both arms: it passes at
this head, and with the base `ApplicationStatus` behaviour restored it fails on
`Always` with `TerminatedWithoutReleaseResources`, message `Application is
terminated without releasing resources as configured.The maximum number of
restart attempts (0) has been exceeded. foo`.
```java
RestartConfig noMoreRestart = new RestartConfig();
noMoreRestart.setRestartPolicy(RestartPolicy.Always);
noMoreRestart.setMaxRestartAttempts(0L);
String exceeded =
String.format(Constants.EXCEED_MAX_RETRY_ATTEMPT_MESSAGE, 0L) + " foo";
for (ResourceRetainPolicy policy : ResourceRetainPolicy.values()) {
ApplicationState state =
status.terminateOrRestart(noMoreRestart, policy, "foo",
false).getCurrentState();
assertEquals(ApplicationStateSummary.ResourceReleased,
state.getCurrentStateSummary());
assertEquals(exceeded, state.getMessage());
}
```
It needs 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]
