dongjoon-hyun commented on PR #851: URL: https://github.com/apache/spark-kubernetes-operator/pull/851#issuecomment-5745997189
Thank you for the detailed review, @peter-toth! I addressed 1, 3 and 5. - **1.** Added `pendingWorkloadKeepsItsPriorityWithoutPermission`. I confirmed it fails when `desired.getSpec().getPriority() != null` is removed and passes as written. - **3.** Added `isPriorityClassChangeAllowed`, which mirrors the CEL rules: while quota is reserved, the presence, the group and the kind of the ref are frozen, and so is the name of a `scheduling.k8s.io` `PriorityClass`. A `WorkloadPriorityClass` name change now goes through, so a Workload waiting for its admission checks can still be raised. `quotaReservedWorkloadKeepsPriorityClass` is split into a frozen case and a mutable one. - **5.** Replaced the private constant with `import static java.net.HttpURLConnection.HTTP_FORBIDDEN`. - **2.** Agreed that the resolution can be gated on a class-name comparison, but it needs `getOrCreateSecondaryResource` split into a get and a create here, which overlaps with #850. Let me do it in a follow-up ticket once #850 is merged. - **4.** I would like to keep the current behavior. Kueue returns the `Get` error for both the `WorkloadPriorityClass` and the pod `PriorityClass` and retries with backoff, so failing one of them with `SchedulingFailure` would deviate from the integration this PR mirrors. The missing user-facing signal is real, and I think the Kueue admission events in #847 are the right place for it. -- 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]
