dongjoon-hyun commented on code in PR #853:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/853#discussion_r4057854409
##########
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:
Your refinement is right and my reply overstated it. With the classification
that landed in c55f3e2, transient, 500 and 429 all return absent, so a strict
`isMasterRequested` can no longer promote a blip at `:178` to
`SchedulingFailure` — the only throw left there is a refused read. My
"transient blip becomes terminal" line was true of 7989aba and is not true of
the current head.
The asymmetry you describe is the sharper version of the argument: a 403 on
the `StatefulSet` read at `:178` would land in the `catch` at `:150` and become
terminal, while a 403 on the `Workload` read a few lines later is caught by
`holdForKueueAdmission` and retried. Same failure, same reconcile, opposite
outcomes. That is what the follow-up has to reconcile, and hoisting the lookup
out of the `try` is still the shape of it — narrower than I described, but for
a reason that survives the fix rather than one the fix removed.
Keeping `getResourceStrictly` private until then, as we both land on.
##########
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:
The instance-ful-class argument is the strongest one against dropping the
rule, and I had not weighted it properly — removing `GodClass` from the ruleset
does retire it everywhere, including the classes where all three terms mean
something, to fix a class where one of them does not. That is a worse trade
than I implied.
So I am no longer leaning either way with confidence. I will decide it in
the follow-up rather than here, where whichever way it goes is a change to the
whole repository's lint policy and does not belong in a bug fix. The split
stands on its own merits regardless of what the linter ends up saying, which is
the part we agree on.
--
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]
