dongjoon-hyun commented on code in PR #853:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/853#discussion_r4057713175
##########
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(
+ final KubernetesClient client, final T desired) {
+ try {
+ return Optional.ofNullable(client.resource(desired).get());
} catch (KubernetesClientException e) {
if (e.getCode() == HTTP_NOT_FOUND) {
return Optional.empty();
}
+ if (isTransientError(e) || e.getCode() == HTTP_INTERNAL_ERROR) {
Review Comment:
Thanks, applied in c55f3e2 — exactly the clause you suggested.
You are right that the inconsistency was already inside this method: the
create side has its own 429 branch and `shouldBackoffBeforeRetry` returns true
for it, so the read side rethrowing a throttle contradicted the rest of the
file. A deferral is not a refusal.
I took your point about the test name too and went with
`createsResourceWhenInitialReadFailsRetriably`, which drops the "reach the API
server" claim that was already inaccurate for 500. Happy to switch to your
`...GetsNoTrustworthyAnswer` if you prefer it. 429 is in the `@ValueSource`,
and the case fails against the previous revision.
While re-checking this branch I also fixed two things in the same commit
that the 429 change touched:
- The warn in that branch used to say "Failed to reach the API server to
read the resource", which was already wrong for 500 and would have been wrong
for 429 as well. It now says the read failed and the resource is treated as
absent, and the lenient wrapper's message names the refusal so the two stay
distinguishable.
- `keepsRetryPathReadLenientOnFailure` was vacuous: it injected 503 on the
second `get()`, but 503 is swallowed by the strict read itself, so the lenient
wrapper it claims to pin was never exercised. Replacing `getResource` with
`getResourceStrictly` at both retry-path call sites left the whole module green
at 300 tests. It now injects 403, and that mutation fails.
--
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]
