dongjoon-hyun commented on code in PR #853:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/853#discussion_r4057713884
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -181,24 +187,54 @@ public static void addOwnerReferenceSecondaryResource(
}
/**
- * Retrieves a Kubernetes resource by its desired state.
+ * Retrieves a Kubernetes resource by its desired state, reporting a
resource that could not be
+ * read as absent.
*
* @param client The KubernetesClient.
* @param desired The desired state of the resource.
* @param <T> The type of the resource, extending HasMetadata.
- * @return An Optional containing the retrieved resource, or empty if not
found.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not readable.
*/
public static <T extends HasMetadata> Optional<T> getResource(
final KubernetesClient client, final T desired) {
- T resource = null;
try {
- resource = client.resource(desired).get();
+ return getResourceStrictly(client, desired);
+ } catch (KubernetesClientException e) {
+ log.warn("Failed to read the resource with responseCode={}, considering
it absent.",
+ e.getCode(), e);
+ return Optional.empty();
+ }
+ }
+
+ /**
+ * Retrieves a Kubernetes resource by its desired state, telling a missing
resource apart from a
+ * read the API server refused. A transient failure keeps reporting the
resource as absent, since
+ * the request did not reach a healthy API server and the create path, which
re-reads on an
+ * AlreadyExists conflict, still resolves the actual state.
+ *
+ * @param client The KubernetesClient.
+ * @param desired The desired state of the resource.
+ * @param <T> The type of the resource, extending HasMetadata.
+ * @return An Optional containing the retrieved resource, or empty if not
found or not reachable.
+ * @throws KubernetesClientException if the API server refused the read.
+ */
+ private static <T extends HasMetadata> Optional<T> getResourceStrictly(
Review Comment:
Agreed on the substance — `isMasterRequested` reading a refused
`StatefulSet` read as "the master was never requested" is the same conflation
this PR removes, and the suspend branch releasing a live master's Kueue
`Workload` is the harmful form of it. I will consider swapping it over in a
follow-up.
One correction on the justification, because it changes what that follow-up
has to do. `isMasterRequested` has two call sites and the `try` starts at
`ClusterInitStep.java:87`, below the first of them:
- The suspend branch at `ClusterInitStep.java:71` — the one you say matters,
and I agree — is **outside** that `try`. A throw there leaves `reconcile()`
altogether and is retried by JOSDK, not written as `SchedulingFailure`.
- The Kueue-hold site at `ClusterInitStep.java:178` is inside it, so a throw
there does reach the `catch (Exception e)` at `ClusterInitStep.java:150`. That
is not "no new handling", though: the javadoc right above it at
`ClusterInitStep.java:169` says *"SchedulingFailure is terminal for a cluster,
so an API failure of the admission request is retried instead"*. Making the
read strict without restructuring would promote a transient blip at that site
to exactly the terminal state that comment exists to avoid.
So the follow-up needs the lookup hoisted out of the `try` and computed
once, rather than a drop-in replacement — worth noting since the second site is
idempotent and would otherwise look free to change. Keeping
`getResourceStrictly` private for now on the same reasoning: I would rather
widen the visibility in the change that actually needs it than expose it
speculatively here.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/utils/ReconcilerUtils.java:
##########
@@ -55,6 +55,7 @@
/** Utility class for reconciler operations. */
@Slf4j
+@SuppressWarnings("PMD.GodClass")
Review Comment:
Thanks for measuring it independently — that matches what I got (base at 47,
this PR to 50).
I take the point that a class-scoped suppression turns the rule off for
whatever is added next. I am leaning the other way, though: dropping `GodClass`
from `config/pmd/ruleset.xml` entirely rather than carrying per-class
suppressions. The ruleset already excludes `CyclomaticComplexity`,
`NPathComplexity`, `CognitiveComplexity` and `TooManyMethods`, so `GodClass` is
the last complexity metric standing, and on a final class of static helpers the
`TCC` term it depends on is not measuring anything real — these methods have no
instance state to share, so the number you quoted is close to what any
stateless utility would score.
That said, the split you describe is worth doing on its own merits,
independent of what the linter says. Let me think about which way to go and I
will follow up.
--
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]
