You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jiangzho (via GitHub)" <gi...@apache.org> on 2026/06/16 23:18:01 UTC
[PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
jiangzho opened a new pull request, #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711
### What changes were proposed in this pull request?
In AppInitStep.reconcile(), when a previous attempt summary exists, the restart backoff
check now handles the case where previousAttemptSummary.getStateTransitionHistory() is
null (non-trim mode). Instead of dereferencing it directly (causing an NPE), the code
falls back to the main status.getStateTransitionHistory() and selects the state entered
immediately before the current initializing state — the stopping state that triggered the
restart — using NavigableMap.lowerEntry().
A null guard for getRestartConfig() was also added to the same block.
### Why are the changes needed?
In non-trim mode (TRIM_ATTEMPT_STATE_TRANSITION_HISTORY=false),
ApplicationStatus.terminateOrRestart() sets previousAttemptSummary to the old
currentAttemptSummary, which always carries stateTransitionHistory = null. The previous
code called .getStateTransitionHistory().get(...) unconditionally, throwing a
NullPointerException on every restart attempt in non-trim mode. This caused the operator
to fail to re-initialize the driver after a restart.
### Does this PR introduce any user-facing change?
No functional behavior change for users running in trim mode (the default). In non-trim
mode, restart backoff is now correctly enforced instead of crashing with an NPE.
### How was this patch tested?
Two unit tests were added to AppInitStepTest:
- nonTrimModeRestartBackoffElapsedProceedsToDriverCreation: verifies that when the backoff has elapsed, the app reaches DriverRequested state without throwing an NPE (non-trim mode, previousAttemptSummary present with null stateTransitionHistory).
- nonTrimModeRestartBackoffActiveRequeuesWithDelay: verifies that when the backoff has not yet elapsed, the reconcile returns a delayed requeue without throwing an NPE, and the state remains ScheduledToRestart.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711#discussion_r3427085231
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -66,17 +70,30 @@ public ReconcileProgress reconcile(
if (app.getStatus().getPreviousAttemptSummary() != null) {
Instant lastTransitionTime = Instant.parse(currentState.getLastTransitionTime());
ApplicationAttemptSummary attemptSummary = app.getStatus().getPreviousAttemptSummary();
- ApplicationState lastState = attemptSummary.getStateTransitionHistory()
- .get(attemptSummary.getStateTransitionHistory().lastKey());
- Instant restartTime =
- lastTransitionTime.plusMillis(
- app.getSpec()
- .getApplicationTolerations()
- .getRestartConfig()
- .getEffectiveRestartBackoffMillis(lastState.getCurrentStateSummary()));
- Instant now = Instant.now();
- if (restartTime.isAfter(now)) {
- return ReconcileProgress.completeAndRequeueAfter(Duration.between(now, restartTime));
+ SortedMap<Long, ApplicationState> attemptHistory = attemptSummary.getStateTransitionHistory();
+ final ApplicationState lastState;
+ if (attemptHistory != null && !attemptHistory.isEmpty()) {
+ lastState = attemptHistory.get(attemptHistory.lastKey());
+ } else {
+ // Non-trim mode: previousAttemptSummary carries null stateTransitionHistory.
+ // Fall back to the main history and pick the state entered just before the current
+ // initializing state (which is the stopping state that triggered the restart).
+ NavigableMap<Long, ApplicationState> mainNav =
+ (NavigableMap<Long, ApplicationState>) app.getStatus().getStateTransitionHistory();
+ Map.Entry<Long, ApplicationState> prevEntry = mainNav.lowerEntry(mainNav.lastKey());
Review Comment:
Nit: This "entry immediately before the last main-history entry" computation already exists in `AppCleanUpStep.getLastObservedStateBeforeTermination` (`AppCleanUpStep.java:300-309`) — same unchecked `NavigableMap` cast, same `lowerEntry`. The two will drift over time. Consider promoting a single guarded helper onto `ApplicationStatus` and calling it from both sites:
```java
/** State entered immediately before the current (last) one, or the current state if none. */
public ApplicationState getStateBeforeCurrent() {
NavigableMap<Long, ApplicationState> nav =
(NavigableMap<Long, ApplicationState>) stateTransitionHistory;
Map.Entry<Long, ApplicationState> prev = nav.lowerEntry(nav.lastKey());
return prev != null ? prev.getValue() : nav.lastEntry().getValue();
}
```
Then this branch becomes `lastState = app.getStatus().getStateBeforeCurrent();`. As a bonus it hardens the `AppCleanUpStep` copy, which dereferences `lowerEntry(...).getValue()` with no null guard. Counter-argument: it couples two reconcile steps to one status method — but they're already coupled by the duplicated logic, just implicitly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711#issuecomment-4744264186
Thank you, @jiangzho and Peter.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711#issuecomment-4744260912
Merged to main
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #711: [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null
URL: https://github.com/apache/spark-kubernetes-operator/pull/711
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711#discussion_r3430347708
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -66,17 +70,30 @@ public ReconcileProgress reconcile(
if (app.getStatus().getPreviousAttemptSummary() != null) {
Instant lastTransitionTime = Instant.parse(currentState.getLastTransitionTime());
ApplicationAttemptSummary attemptSummary = app.getStatus().getPreviousAttemptSummary();
- ApplicationState lastState = attemptSummary.getStateTransitionHistory()
- .get(attemptSummary.getStateTransitionHistory().lastKey());
- Instant restartTime =
- lastTransitionTime.plusMillis(
- app.getSpec()
- .getApplicationTolerations()
- .getRestartConfig()
- .getEffectiveRestartBackoffMillis(lastState.getCurrentStateSummary()));
- Instant now = Instant.now();
- if (restartTime.isAfter(now)) {
- return ReconcileProgress.completeAndRequeueAfter(Duration.between(now, restartTime));
+ SortedMap<Long, ApplicationState> attemptHistory = attemptSummary.getStateTransitionHistory();
+ final ApplicationState lastState;
+ if (attemptHistory != null && !attemptHistory.isEmpty()) {
+ lastState = attemptHistory.get(attemptHistory.lastKey());
+ } else {
+ // Non-trim mode: previousAttemptSummary carries null stateTransitionHistory.
+ // Fall back to the main history and pick the state entered just before the current
+ // initializing state (which is the stopping state that triggered the restart).
+ NavigableMap<Long, ApplicationState> mainNav =
+ (NavigableMap<Long, ApplicationState>) app.getStatus().getStateTransitionHistory();
+ Map.Entry<Long, ApplicationState> prevEntry = mainNav.lowerEntry(mainNav.lastKey());
+ lastState = prevEntry != null ? prevEntry.getValue() : mainNav.lastEntry().getValue();
+ }
+ RestartConfig restartConfig =
+ app.getSpec().getApplicationTolerations().getRestartConfig();
+ if (restartConfig != null) {
Review Comment:
May I ask when `restartConfig == null` here?
If it doesn't happen in the context, can we simply do some assertion statement for this condition instead of `if (restartConfig != null)`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57497] Fix NPE in AppInitStep restart backoff check when stateTransitionHistory is null [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #711:
URL: https://github.com/apache/spark-kubernetes-operator/pull/711#discussion_r3427085231
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -66,17 +70,30 @@ public ReconcileProgress reconcile(
if (app.getStatus().getPreviousAttemptSummary() != null) {
Instant lastTransitionTime = Instant.parse(currentState.getLastTransitionTime());
ApplicationAttemptSummary attemptSummary = app.getStatus().getPreviousAttemptSummary();
- ApplicationState lastState = attemptSummary.getStateTransitionHistory()
- .get(attemptSummary.getStateTransitionHistory().lastKey());
- Instant restartTime =
- lastTransitionTime.plusMillis(
- app.getSpec()
- .getApplicationTolerations()
- .getRestartConfig()
- .getEffectiveRestartBackoffMillis(lastState.getCurrentStateSummary()));
- Instant now = Instant.now();
- if (restartTime.isAfter(now)) {
- return ReconcileProgress.completeAndRequeueAfter(Duration.between(now, restartTime));
+ SortedMap<Long, ApplicationState> attemptHistory = attemptSummary.getStateTransitionHistory();
+ final ApplicationState lastState;
+ if (attemptHistory != null && !attemptHistory.isEmpty()) {
+ lastState = attemptHistory.get(attemptHistory.lastKey());
+ } else {
+ // Non-trim mode: previousAttemptSummary carries null stateTransitionHistory.
+ // Fall back to the main history and pick the state entered just before the current
+ // initializing state (which is the stopping state that triggered the restart).
+ NavigableMap<Long, ApplicationState> mainNav =
+ (NavigableMap<Long, ApplicationState>) app.getStatus().getStateTransitionHistory();
+ Map.Entry<Long, ApplicationState> prevEntry = mainNav.lowerEntry(mainNav.lastKey());
Review Comment:
Nit: This "entry immediately before the last main-history entry" computation already exists in `AppCleanUpStep.getLastObservedStateBeforeTermination` (`AppCleanUpStep.java:300-309`) — same unchecked `NavigableMap` cast, same `lowerEntry`. The two will drift over time. Consider promoting a single guarded helper onto `ApplicationStatus` and calling it from both sites:
```java
/** State entered immediately before the current (last) one, or the current state if none. */
public ApplicationState getStateBeforeCurrent() {
NavigableMap<Long, ApplicationState> nav =
(NavigableMap<Long, ApplicationState>) stateTransitionHistory;
Map.Entry<Long, ApplicationState> prev = nav.lowerEntry(nav.lastKey());
return prev != null ? prev.getValue() : nav.lastEntry().getValue();
}
```
Then this branch becomes `lastState = app.getStatus().getStateBeforeCurrent();`. As a bonus it hardens the `AppCleanUpStep` copy, which dereferences `lowerEntry(...).getValue()` with no null guard.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org