dongjoon-hyun commented on PR #857:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5762533565
Thanks, @peter-toth! Findings 5 and 6 are addressed in `a6c5ec6`, and 7 is
left for a follow-up as you offered.
**5 (Blocking)** — Folded the lenient removal in, as you suggested. With the
wrapper gone there is only one variant left, so the `Strictly` suffix has
nothing to contrast with and the method takes the plain
`getCurrentAttemptDriverPod` name back. That keeps the six behaviour cases in
`SparkAppContextTest` as they are, now pointed at the method production
actually calls, and turns `apiErrorDuringVerificationIsTreatedAsAbsent` into
`apiErrorDuringVerificationIsPropagated` with the `assertThrows` you wrote.
I ran your mutant both ways to confirm the hole is closed: with the body
catching `KubernetesClientException` and returning `Optional.empty()`,
`SparkAppContextTest.apiErrorDuringVerificationIsPropagated` fails, and it
passes on the restored source. `SparkAppContext` also loses its `@Slf4j`, since
the wrapper held its only log call.
**6 (Non-blocking)** — Added `retryAfterRequestFailure` next to
`holdForAdmission` and pointed all three catches at it, which takes the two
init-step catches from 21 lines to 8 each. The `IllegalStateException` arm of
`holdForAdmission` stays as it is, since a malformed Workload is not
classified. Two adjustments to your snippet:
- It has to be `public static`. Both callers are in
`reconciler.reconcilesteps`, so package-private does not compile — the same
shape as finding 2, one package up.
- The extra method puts `KueueWorkloadUtils` at `WMC=47`, so PMD's
`GodClass` trips. I annotated the class with
`@SuppressWarnings("PMD.GodClass")`, following what SPARK-59667 did to
`ReconcilerUtils` one commit earlier for the same reason.
**7 (Non-blocking)** — Agreed, and thanks for raising it here rather than
against #854. Since `AppInitStep.java:78-84` has the same gap and a new
`SUSPEND_CHECK_FAILED` reason is user-facing on its own, I would rather add the
reason, both call sites and the `docs/configuration.md` row in one follow-up
than widen this PR, which is already two rebases past what it started as.
--
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]
